Skip to content
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

fix: return nil for non-existant key in AwsSdk::MessageAttributeGetter #853

Merged

Conversation

yoheyk
Copy link
Contributor

@yoheyk yoheyk commented Feb 7, 2024

Since tracestate is optional, it can be omitted.

However, current implementation of AwsSdk::MessageAttributeGetter raises an exception here when it is omitted.

Same as TextMapGetter, it should return nil for non-existant key.

@@ -29,7 +29,8 @@ def self.set(carrier, key, value)
# OpenTelemetry.propagation.extract(message, getter: MessageAttributeGetter)
class MessageAttributeGetter
def self.get(carrier, key)
carrier[key][:string_value] if carrier[key][:data_type] == 'String'
message_attribute = carrier[key]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, just a little nit.
Would return unless carrier[key] introduce less code change?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it is less code change but since it is doing carrier[key] multiple times, it seems to be a little slower:

def get_with_variable(carrier, key)
  message_attribute = carrier[key]
  message_attribute[:string_value] if message_attribute && message_attribute[:data_type] == 'String'
end

def get_with_return(carrier, key)
  return unless carrier[key]

  carrier[key][:string_value] if carrier[key][:data_type] == 'String'
end

carrier = { 'traceparent' => { data_type: 'String', string_value: 'tp' } }

Benchmark.ips do |x|
  x.report("get_with_variable") { get_with_variable(carrier, 'traceparent') }
  x.report("get_with_return") { get_with_return(carrier, 'traceparent') }

  x.compare!
end
Calculating -------------------------------------
   get_with_variable      4.959M (± 7.8%) i/s -     24.754M in   5.053304s
     get_with_return      4.337M (± 2.0%) i/s -     21.870M in   5.045064s

Comparison:
   get_with_variable:  4959117.6 i/s
     get_with_return:  4336852.7 i/s - 1.14x  slower

but either way is fine. let me know if I should change the code

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for putting up the comparison. I am ok with better performance.

@arielvalentin
Copy link
Collaborator

@yoheyk Thank you for your contribution.

We are currently looking for help maintaining AWS related instrumentation.

Would you be interested?

@arielvalentin arielvalentin merged commit 85c7f5c into open-telemetry:main Feb 8, 2024
50 checks passed
@yoheyk yoheyk deleted the aws-message-attribute-getter-error branch February 8, 2024 23:28
@yoheyk
Copy link
Contributor Author

yoheyk commented Feb 8, 2024

Thanks for merging and releasing so quickly!

We are currently looking for help maintaining AWS related instrumentation.

Would you be interested?

@arielvalentin yeah, I think I can help to some extent.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants