Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
End to end tracing support - Lambda #983
End to end tracing support - Lambda #983
Changes from 55 commits
dcc01de
1dd54d9
03f6fb4
5919be5
ebd45b4
e1ff7d0
041d9b8
e12a4b1
e906d2a
86c04d1
047f5d0
21db8fa
5204d27
c9c1bca
c956d4b
12c6c74
e042a6f
1f6b745
76a52a1
f0ec58b
f048e4b
23638fb
439023e
f7ac5db
e932685
8c86bb4
debba6c
d481dcb
fedb803
38a9e91
6cc43b2
f0b4a7f
6df424c
5b70dfe
9de0d34
51144f5
8f8e491
d3d2c18
e572b7b
ac8b307
24c9108
7b4a00e
2c391fc
b64863e
e463802
804939b
0f51b45
8226dd3
986e366
af8a871
8e2c1de
8c53555
e2a18cc
ada0137
1a0457f
1754a23
8445736
c0c6f30
92b045b
4df95c7
93e9916
bd387fd
cbc1242
8ee0d8a
8dd0fe8
fa51016
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does the example need either
otel.SetTracerProvider(tp)
or passingtp
explicitly into the Lambda handler? Otherwise the variable is unused.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, made a suggestion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this be solved through lazy initialization?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not without asking the user to add more code. The
faas.id
andcloud.account.id
attributes are only available inside of the handler, and our solution is meant to be strictly a wrapper of the handler such that we can't actually do anything inside the wrapper. So we could ask the user to lazy-initialize the tracer provider in their handler, but that's a much worse experience.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why would the user have to do that lazy-initialization, rather than the framework? E.g., at this point it seems like you'd have access to those invocation-scoped variables. It's true that you'd have to do separately for each possible reflective type of the handler function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the problem is that we will then couple the Lambda resource detector and the Lambda instrumentation, which ought to be two separate components. The Lambda instrumentation does indeed have access to those invocation-scoped attributes, and is able to initialize the tracer provider. But the Lambda resource detector must operate only with what is available in the static environment, so if a user wants to initialize a custom tracer provider outside their handler as seen in the example it will work the same.
I agree that it's unfortunate the lambda resource detector doesn't capture these attributes, but I believe this is a limitation of Lambda itself more than any individual instrumentation, which is why the span attribute is called out in the spec and there's talks of not even marking faas.id as a resource attribute at all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for explaining. I was commenting based on "what would an operator find most useful" vs "what is a clean way to factor the code." Given the ambiguity / ongoing discussion around faas.id, I think this code makes a reasonable choice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please update the versions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bhautikpip if you have a minute would you be able to update these versions to stable and rebase the PR so conflicts/CI checks are resolved?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@codeboten @willarmiros updated the version and resolved the conflicts. It should pass the CI checks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!