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

Message handler #8331

Draft
wants to merge 48 commits into
base: main
Choose a base branch
from
Draft

Message handler #8331

wants to merge 48 commits into from

Conversation

atshaw43
Copy link

Problem
Adding Message Handler for messaging systems.
These new handlers will create a span for the user and add span links and set the messaging attributes.

Testing
Unit tests

@atshaw43 atshaw43 requested a review from a team April 21, 2023 07:04
@github-actions github-actions bot requested a review from theletterf April 21, 2023 07:04
@laurit
Copy link
Contributor

laurit commented Apr 21, 2023

@mateuszrzeszutek mateuszrzeszutek added the needs author feedback Waiting for additional feedback from the author label Apr 21, 2023
@atshaw43
Copy link
Author

atshaw43 commented Apr 21, 2023

@atshaw43 we already have aws lambda instrumentation in https://github.com/open-telemetry/opentelemetry-java-instrumentation/tree/main/instrumentation/aws-lambda/aws-lambda-events-2.2/library

Those are Lambda specific because of the Context variable. This is for non-Lambda cases such as EC2.

This is for batching messages, while those classes are for for-loop cases. One message at a time.

@github-actions github-actions bot removed the needs author feedback Waiting for additional feedback from the author label Apr 21, 2023
@atshaw43
Copy link
Author

@laurit
Bump

@laurit
Copy link
Contributor

laurit commented Apr 26, 2023

@atshaw43 Firstly the module name message-handler doesn't seem appropriate as the code seems to deal only with aws messages. Perhaps it should be named aws-something-something or even be moved to aws-sdk module. Secondly instrumentations in this repository must use instrumenter api. The provided documentation does not do a good job illustrating why this code should be used. Also I believe aws-sdk instrumentation already creates spans for sqs, can this code be used together with aws-sdk instrumentation?

@Oberon00
Copy link
Member

Oberon00 commented Apr 26, 2023

Firstly the module name message-handler doesn't seem appropriate as the code seems to deal only with aws messages.

I think this is only sort of true. The code is in many places generic, and I think with a few small adjustments, the interface could be fit for any messages. But at the moment, I agree that it is too AWS-specific.

Note that there is also very similar functionality in the AWS Lambda instrumentation, that could be used as inspiration and generalized: https://github.com/open-telemetry/opentelemetry-java-instrumentation/blob/main/instrumentation/aws-lambda/aws-lambda-events-2.2/library/src/main/java/io/opentelemetry/instrumentation/awslambdaevents/v2_2/TracingSqsMessageHandler.java (maybe even in a way that the AWS Lambda instrumentation could re-use code from this new proposed message handler module)

@atshaw43
Copy link
Author

@atshaw43 Firstly the module name message-handler doesn't seem appropriate as the code seems to deal only with aws messages. Perhaps it should be named aws-something-something or even be moved to aws-sdk module. Secondly instrumentations in this repository must use instrumenter api. The provided documentation does not do a good job illustrating why this code should be used. Also I believe aws-sdk instrumentation already creates spans for sqs, can this code be used together with aws-sdk instrumentation?

@laurit
Thanks for the feedback.
I have moved the x-ray specific code to the SqsMessageHandler.

@atshaw43 atshaw43 marked this pull request as draft June 16, 2023 16:37
@atshaw43
Copy link
Author

atshaw43 commented Jul 21, 2023

I updated the PR so the MessageHandler focuses on process spans.
I had originally tried to have it handle receive and process spans, but that is not feasible.
I will open a separate PR for auto-instrumentation for SQS receive message nodes.

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.

5 participants