feat: SQS / SNS messaging attributes and context propagation #1026

YanivD · 2021-11-21T15:53:09Z

What's added in this PR?

Added message attributes for SQS and SNS services
Added context propagation support for SQS and SNS. For SQS.SendMessage, SQS.SendMessageBatch and SNS.Publish methods

Context propagation strategy

Currently SQS doesn't natively support propagating the OTEL context.
I used JS aws-sdk instrumentation as an inspiration for this PR. It use message metadata to populate propagator fields.

Limitations

SQS support up to 10 message metadata attributes. Context attributes will not be added if there are too many attributes.

What's missing?

Currently context propagation is working only if the receiver is using JS aws-sdk. I'm planning to add support in another separated PR.

Note: This PR contain file changes from #1023

ericmustin · 2021-11-21T18:21:39Z

@mottibec @blumamir @YanivD I'm not quite sure what these approvals from your colleagues are supposed to indicate. The work and motivation is absolutely appreciated, but there are currently multiple errors in this instrumentation and the test suite is clearly and loudly failing and some changes are still required. Approving PRs without reviewing them doesn't do anything except add noise, and erode confidence that you and your colleagues are carefully implementing and testing the changes in this Pull Request. I fully understand that you have product/professional obligations which rely on successfully merging and releasing this work, and I am scheduling time to review this this week. Thank you for your patience and for all the great work so far.

YanivD · 2021-11-21T18:50:19Z

@ericmustin
We're excited for the opportunity to contribute and help the community. We didn't mean to make unnecessary noises.
Folks, we really appreciate all your help, guidance and your professional quick response.
I'll address the failing tests soon and make sure this pr is ready for review. Until then, marking it as draft. Any review and suggestions is welcome.

…uby into aws-sdk-messaging

This reverts commit 71dd268.

YanivD · 2021-11-22T07:54:30Z

Updated the pr, now the tests are green and ready for review.
The failing tests were from aws-sdk version 2. The context interface changed in version 3. Appraisal tool is great!

instrumentation/aws_sdk/lib/opentelemetry/instrumentation/aws_sdk/messaging_helper.rb

YanivD · 2021-12-19T11:00:49Z

Thanks @ericmustin
After revisiting v2.0 support, I modified service_name extraction to support 2.0 versions - 77074a1.
This PR won't change the supported versions.

@open-telemetry/ruby-maintainers
Please let me know if more changes are required

ericmustin · 2021-12-19T18:41:12Z

@YanivD lgtm, ty for addressing that, will try to get some other eyes on it from approvers/maintainers this week. Just for reference in US/CA we're a bit light this week and next bc of Christmas holidays, apols if there's any delay here. 🎅 😅

YanivD · 2021-12-20T14:22:57Z

I want to update that I'm working on extracting SQS context, for creating 'process' spans.
Do you prefer I'll add the extraction part to this PR? Or to create separated PR for that?

mwear

I have two minor comments / suggestions, otherwise, LGTM.

instrumentation/aws_sdk/lib/opentelemetry/instrumentation/aws_sdk/messaging_helper.rb

instrumentation/aws_sdk/lib/opentelemetry/instrumentation/aws_sdk/handler.rb

…sdk/messaging_helper.rb Co-authored-by: Matthew Wear <matthew.wear@gmail.com>

ericmustin · 2021-12-22T14:47:00Z

@YanivD I'd say lets move the extract stuff into a seperate PR if possible so we can get this chunk of work merged (i see you've already pushed up some commits, apologies for the delayed response), the extract stuff is probably going to take a bit of time to review and get feedback for, and i don't want that to block the work you've already done here from getting released

YanivD · 2022-01-12T07:54:48Z

@open-telemetry/ruby-maintainers
Is there anything left to do before merging this PR to main branch?

habmic · 2022-01-18T15:57:34Z

Do you know when this PR will be merged?

ericmustin · 2022-01-18T15:58:38Z

@habmic @YanivD ack'd, will make sure this gets surfaced at SIG today

fbogsany · 2022-01-18T16:03:56Z

instrumentation/aws_sdk/lib/opentelemetry/instrumentation/aws_sdk/handler.rb

          return super unless context

-          service_name = context.client.class.api.metadata['serviceId'] || context.client.class.to_s.split('::')[1]
+          service_name = context.client.class.api.metadata['serviceId'] || context.client.class.to_s.split('::')[1] rescue context.client.class.to_s.split('::')[1] # rubocop:disable Style/RescueModifier


What is this intended to do? The rescue clause contains the same expression as the right side of ||, so if that raised an exception, then the rescue clause will as well.

Updated service_name extraction 1a59955

This change was to support 2.0.x versions. When using 2.0.x, metadata method expect an argument, what raised an exception.

Please review this change and approved it's safe.

instrumentation/aws_sdk/lib/opentelemetry/instrumentation/aws_sdk/messaging_helper.rb

…nto aws-sdk-messaging

nvolker · 2022-04-13T06:58:36Z

@YanivD just a question on the "What's Missing" part of your description. Specifically "I'm planning to add support in another separated PR", is this to do with extractor propagation on the receiver side? And are you actively looking into this?

I'm currently going with the following configuration and building out my own bespoke propagation extractor to allow the trace to continue in a consuming service:

  c.use "OpenTelemetry::Instrumentation::AwsSdk", {
    inject_messaging_context: true,
    # suppress_internal_instrumentation: true # I thought I'd want this but I don't, as it still sends `receive` spans
  }

If you have any advice for the interim, that would be appreciated! Really appreciate the work you've done here thus far.

YanivD · 2022-04-13T13:27:35Z

@nvolker
I have implemented the extraction part on my fork, it's still a draft and not fully ready to review. You can see it here - YanivD#12

If you have the time to finish the changes and open PR - it will really help and you are very welcome :)
Otherwise, I'll probably get to it and the next few weeks and will open a PR

robertlaurin and others added 8 commits November 18, 2021 20:55

fix: aws-sdk nitpicks

837998e

implement sqs/sns context propagation

5f9eac1

apply message attributes for SQS/SNS

2464f6c

using client_method instead of operation

f51bec2

fix context propagation for sqs send batch

ca6b7e4

add tests for sns & sqs services

49913a3

cover MessageAttributeSetter with tests

b23ede3

aws-sdk test - ignore OpenTelemetry logs

2ce5567

YanivD requested review from ahayworth, arielvalentin, dazuma, ericmustin, fbogsany, mwear and robertlaurin as code owners November 21, 2021 15:53

Merge branch 'main' into aws-sdk-messaging

23d33ac

mottibec approved these changes Nov 21, 2021

View reviewed changes

rubocop fix

f1abdcc

YanivD marked this pull request as draft November 21, 2021 18:50

Yaniv Davidi added 4 commits November 22, 2021 08:21

Merge branch 'aws-sdk-messaging' of github.com:YanivD/opentelemetry-r…

8d3f7b5

…uby into aws-sdk-messaging

enable github actions on fork repo

71dd268

support context data structure for older versions

dc44180

Revert "enable github actions on fork repo"

2a2dfce

This reverts commit 71dd268.

YanivD marked this pull request as ready for review November 22, 2021 07:54

arielvalentin reviewed Nov 22, 2021

View reviewed changes

instrumentation/aws_sdk/lib/opentelemetry/instrumentation/aws_sdk/messaging_helper.rb Outdated Show resolved Hide resolved

Yaniv Davidi added 2 commits November 23, 2021 12:54

fix aws-sdk status message in case of an error

be98298

remove unnecessary rubocop disable

b5ef80c

Merge branch 'main' into aws-sdk-messaging

ea5af80

mwear approved these changes Dec 21, 2021

View reviewed changes

instrumentation/aws_sdk/lib/opentelemetry/instrumentation/aws_sdk/messaging_helper.rb Outdated Show resolved Hide resolved

instrumentation/aws_sdk/lib/opentelemetry/instrumentation/aws_sdk/handler.rb Outdated Show resolved Hide resolved

YanivD and others added 3 commits December 22, 2021 08:35

Update instrumentation/aws_sdk/lib/opentelemetry/instrumentation/aws_…

2e6189e

…sdk/messaging_helper.rb Co-authored-by: Matthew Wear <matthew.wear@gmail.com>

fix queue_name method

388a00c

use private const SEND_MESSAGE_CLIENT_METHODS

431deae

Merge branch 'main' into aws-sdk-messaging

b55eb93

Merge branch 'main' into aws-sdk-messaging

7a44b8d

fbogsany reviewed Jan 18, 2022

View reviewed changes

instrumentation/aws_sdk/lib/opentelemetry/instrumentation/aws_sdk/messaging_helper.rb Outdated Show resolved Hide resolved

Yaniv Davidi added 4 commits January 19, 2022 10:03

Merge branch 'main' of github.com:open-telemetry/opentelemetry-ruby i…

c650188

…nto aws-sdk-messaging

chore(aws_sdk): improve getting queue name

5f34bc0

chore(aws-sdk): improve service_name extraction

1a59955

ignore rubocop error

13ba36a

fbogsany approved these changes Jan 19, 2022

View reviewed changes

Merge branch 'main' into aws-sdk-messaging

72d2ebd

ericmustin approved these changes Jan 19, 2022

View reviewed changes

fbogsany merged commit 658440f into open-telemetry:main Jan 19, 2022

YanivD deleted the aws-sdk-messaging branch April 13, 2022 13:54

This was referenced May 9, 2022

feat: SQS create process spans YanivD/opentelemetry-ruby#12

Draft

feat: SQS create process spans #1248

Closed

plantfansam mentioned this pull request Jun 17, 2022

Sqs processing spans open-telemetry/opentelemetry-ruby-contrib#51

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: SQS / SNS messaging attributes and context propagation #1026

feat: SQS / SNS messaging attributes and context propagation #1026

YanivD commented Nov 21, 2021

ericmustin commented Nov 21, 2021

YanivD commented Nov 21, 2021 •

edited

Loading

YanivD commented Nov 22, 2021

YanivD commented Dec 19, 2021

ericmustin commented Dec 19, 2021

YanivD commented Dec 20, 2021

mwear left a comment

ericmustin commented Dec 22, 2021

YanivD commented Jan 12, 2022

habmic commented Jan 18, 2022

ericmustin commented Jan 18, 2022

fbogsany Jan 18, 2022

YanivD Jan 19, 2022 •

edited

Loading

nvolker commented Apr 13, 2022 •

edited

Loading

YanivD commented Apr 13, 2022

feat: SQS / SNS messaging attributes and context propagation #1026

feat: SQS / SNS messaging attributes and context propagation #1026

Conversation

YanivD commented Nov 21, 2021

What's added in this PR?

Context propagation strategy

Limitations

What's missing?

ericmustin commented Nov 21, 2021

YanivD commented Nov 21, 2021 • edited Loading

YanivD commented Nov 22, 2021

YanivD commented Dec 19, 2021

ericmustin commented Dec 19, 2021

YanivD commented Dec 20, 2021

mwear left a comment

Choose a reason for hiding this comment

ericmustin commented Dec 22, 2021

YanivD commented Jan 12, 2022

habmic commented Jan 18, 2022

ericmustin commented Jan 18, 2022

fbogsany Jan 18, 2022

Choose a reason for hiding this comment

YanivD Jan 19, 2022 • edited Loading

Choose a reason for hiding this comment

nvolker commented Apr 13, 2022 • edited Loading

YanivD commented Apr 13, 2022

YanivD commented Nov 21, 2021 •

edited

Loading

YanivD Jan 19, 2022 •

edited

Loading

nvolker commented Apr 13, 2022 •

edited

Loading