Update Python Sample Apps for Upstream 1.3 #110
Conversation
NathanielRN
force-pushed
the
python-apps-1.3-update
branch
4 times, most recently
from
88dfb00 to
3d17ddf
Compare
NathanielRN
force-pushed
the
python-apps-1.3-update
branch
from
3d17ddf to
6ba9b00
Compare
NathanielRN
requested review from
anuraaga,
codeboten and
wangzlei
as code owners
| @@ -30,7 +30,7 @@ | |||
| ) | |||
|
|
|||
| # Enable instrumentation | |||
| AwsLambdaInstrumentor().instrument() | |||
| AwsLambdaInstrumentor().instrument(skip_dep_check=True) | |||
There was a problem hiding this comment.
Can we use flag skip_dep_check to remove these code ?
There was a problem hiding this comment.
Hm I'm not sure what we would replace it with. The _load_distros, _load_instrumentors and _load_configurators all initialize the classes they find. Normally, its important to not skip_dep_check because the classes _load_instrumentors loads requires that the dependencies are installed. Attempting to instrument without the dependencies would cause an import error.
I think it might be possible to use the upstream opentelemetry-instrumentation package as it is today instead of doing this code ourselves, but we don't need that in this PR.
There was a problem hiding this comment.
Let's merge the code first. I feel if is_recording() is essential why it is not integrated inside of start_span. I can raise issue in python repo later.
| span.set_attribute("faas.version", function_version) | ||
|
|
||
| result = original_func(*args, **kwargs) | ||
| if span.is_recording(): |
There was a problem hiding this comment.
I am not sure it's necessary. When start the span and sampled, is_recording() is always true?
There was a problem hiding this comment.
That's a good question! Initially I didn't know why we did this and I just did it because we do it for every other instrumentation as seen in open-telemetry/opentelemetry-python#1057, so I wanted AwsLambdaInstrumentor to be the same.
But now I understand that we need this because we don't know if the Span will be Sampled. It is started like you said but if the Sampler doesn't sample this span then we shouldn't record attributes.
| @@ -30,7 +30,7 @@ | |||
| ) | |||
|
|
|||
| # Enable instrumentation | |||
| AwsLambdaInstrumentor().instrument() | |||
| AwsLambdaInstrumentor().instrument(skip_dep_check=True) | |||
There was a problem hiding this comment.
Let's merge the code first. I feel if is_recording() is essential why it is not integrated inside of start_span. I can raise issue in python repo later.
* Update Python Sample Apps for Upstream 1.3 * Skip dep check lets LambdaInstrumentor get called * Copy style of contrib instrumentations
Description
To make our sample apps work with the 1.3 upstream release of OTel Python, we need to make changes to our sample app for the minor breaking changes.
Testing
I was able to get the correct trace both by executing from the Lambda console and by using
curl -sSto the API gateway link where my lambda layer was being used.Upstream PRs that caused changes