-
Notifications
You must be signed in to change notification settings - Fork 210
fix!: check if span has the attributes method to avoid internal error #1645
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
base: main
Are you sure you want to change the base?
Conversation
@@ -43,7 +43,7 @@ def wrap_lambda(event:, context:, handler:, flush_timeout: DEFAULT_FLUSH_TIMEOUT | |||
begin | |||
response = yield | |||
|
|||
unless span.attributes.key?(OpenTelemetry::SemanticConventions::Trace::HTTP_STATUS_CODE) | |||
if span.respond_to?(:attributes) && !span.attributes.key?(OpenTelemetry::SemanticConventions::Trace::HTTP_STATUS_CODE) |
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.
Probably can just check if the span is recording, right?
if span.respond_to?(:attributes) && !span.attributes.key?(OpenTelemetry::SemanticConventions::Trace::HTTP_STATUS_CODE) | |
if span.recording? && !span.attributes.key?(OpenTelemetry::SemanticConventions::Trace::HTTP_STATUS_CODE) |
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.
How is this span attribute getting set anyways? Instrumentation should really not be making any decisions based of the contents of the spans attributes.
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.
aws lambda instrumentation attribute is set through otel_attributes(event, context)
from in_span; and then it will add the attribute HTTP_STATUS_CODE
.
Instrumentation should really not be making any decisions based of the contents of the spans attributes.
This part only sets attributes like HTTP_STATUS_CODE as part of span metadata.
The issue is that if the span is not recording, it won't have the attributes method, which can raise an error. This error could overshadow the user's original Lambda function error, causing confusion.
…ws_lambda/wrap.rb Co-authored-by: Robert <[email protected]>
thank you for this PR @xuan-cao-swi! This error causes lambda to fail when using the otel lambda layer detailed here. In our case, the request contains traceparent header that says no sampling and the lambda fails and api gateway returns 502. |
Hi @spongenee, thanks for reaching out! Have you given this branch a try? Does it resolve the problem for you? |
I would love to give a try but don't know how. I am currently using the ruby lambda layer, can I build a layer with this branch? |
Yes! You can build a layer with this branch. If you clone the opentelemetry-lambda repository, you can update the Gemfile to install the gem explicitly with a link to this branch. For example, below the gem 'opentelemetry-instrumentation-aws_lambda', github: 'xuan-cao-swi/opentelemetry-ruby-contrib', glob: 'instrumentation/aws_lambda/*.gemspec', branch: 'lambda-inst-fix' After that, I'm not sure if it was just a problem with my machine, but I also needed to update lines 58-60 of the Dockerfile to include
Once the script finishes, you should have a new If that's all more work than you're up for, don't worry about it 🙂 |
thank you very much for the detailed instructions @kaylareopelle! I am able to follow the steps but I get the following error when triggering the function with the layer
I think it makes sense because the following line in the gemfile probably checks the version of
|
Hi @spongenee , one way (very hacky way) is that you can edit this file directly, and then pack everything into zip file. With the zip file, you can upload to layer. After download the otel-lambda, you can navigate to the path |
Description
In rare cases, the span may be a no-op span because it is a non-recording span. If the span attempts to access its attributes, it will raise an error. A check has been added to prevent this internal error.