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

Add support for Micrometer's Observation API for the SQS pipeline #1164

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

sondemar
Copy link
Contributor

@sondemar sondemar commented Jun 10, 2024

📢 Type of change

  • Bugfix
  • New feature
  • Enhancement
  • Refactoring

📜 Description

Added Micrometer's Observability support instruments SQS processing out of the box, while additionally propagating the inbound tracing information by looking it up in MessageHeaders.
Closes #646

💡 Motivation and Context

💚 How did you test it?

📝 Checklist

  • I reviewed submitted code
  • I added tests to verify changes
  • I updated reference documentation to reflect the change
  • All tests passing
  • No breaking changes

🔮 Next steps

@github-actions github-actions bot added component: sqs SQS integration related issue type: documentation Documentation or Samples related issue labels Jun 10, 2024
@MatejNedic MatejNedic added status: team-discussion Team has to figure out how to proceed type: feature Integration with a new AWS service or bigger change in existing integration and removed type: documentation Documentation or Samples related issue labels Jun 12, 2024
@tomazfernandes
Copy link
Contributor

Hi @sondemar, on a first glance this looks great!

I wonder if it'd also make sense to add Observability support to SqsTemplate? Or perhaps leave it to a separate issue / PR?

Let me know your thoughts, thanks!

@sondemar
Copy link
Contributor Author

Hi @sondemar, on a first glance this looks great!

I wonder if it'd also make sense to add Observability support to SqsTemplate? Or perhaps leave it to a separate issue / PR?

Let me know your thoughts, thanks!

Hi @tomazfernandes , I have also thought about templates (SqsTemplate and SnsTemplate as well). It makes sense to include them because we might want to propagate the tracing context while sending messages to SQS/SNS or receiving messages from SQS (SqsTemplate). Initially, I considered including support for SQS/SNS templates in a separate ticket, but including SqsTemplate with this ticket is also fine.

@tomazfernandes
Copy link
Contributor

Hi @tomazfernandes , I have also thought about templates (SqsTemplate and SnsTemplate as well). It makes sense to include them because we might want to propagate the tracing context while sending messages to SQS/SNS or receiving messages from SQS (SqsTemplate). Initially, I considered including support for SQS/SNS templates in a separate ticket, but including SqsTemplate with this ticket is also fine.

Makes sense, always a good idea to break into smaller PRs. In this case, TBH I'm not super familiar with Micrometer 2, so it'd be helpful to have a PR open for SqsTemplate instrumentation as well so I can run them locally and see the propagation happening, if that's ok with you.

@tomazfernandes
Copy link
Contributor

tomazfernandes commented Jun 29, 2024

Another point here is - the most challenging part of instrumenting observation for this project is that it has many points where thread hopping may occur.

The main reason is it supports any combination of Blocking and Non-blocking components such as Interceptors, Message Listener and Error Handlers.

So I believe ideally we would have an Integration Test such as e.g.:

  • An AsyncMessageInterceptor that performs an async action using AsyncSqsClient (that should switch to an AWS SDK thread)
  • A blocking Messageinterceptor that doesn't really have to do anything (that should switch to a thread from the standard / provided Task executor)
  • An async message listener that performs an action using AsyncSqsClient and throws an error (should switch to a AWS SDK Thread)
  • A blocking ErrorHandler that doesn't have to do anything (should switch back to a TaskExecutor thread)

As a user I would expect to have Observability context in all these points, regardless of async / sync components.

Of course, we could start supporting only let's say the message listener, but then we'd need to make it clear in the docs what are the limitations.

Makes sense? Please let me know your thoughts. Thanks!

@sondemar
Copy link
Contributor Author

sondemar commented Jul 5, 2024

I have already started the implementation of observability support for SqsTemplate (in the same PR), but now I am waiting for feedback on the following questions: micrometer-metrics/context-propagation#262 and micrometer-metrics/tracing#765.

Regarding the scenarios for the integration test, it makes sense, and I will apply them.

@tomazfernandes
Copy link
Contributor

Hello @sondemar, just to catch up on this

but now I am waiting for feedback on the following questions: micrometer-metrics/context-propagation#262 and micrometer-metrics/tracing#765.

I see you're still looking into this, anything else we could do on this end?

Thanks!

@sondemar
Copy link
Contributor Author

Hello @sondemar, just to catch up on this

but now I am waiting for feedback on the following questions: micrometer-metrics/context-propagation#262 and micrometer-metrics/tracing#765.

I see you're still looking into this, anything else we could do on this end?

Thanks!

Hi @tomazfernandes !

I have found a solution for the issues I encountered. I am currently completing the integration tests proposed in this comment. I expect to have the full functionality ready by next week.

Thank you for your patience!

@github-actions github-actions bot added the type: documentation Documentation or Samples related issue label Aug 27, 2024
@sondemar sondemar force-pushed the micrometer-observation-support branch 4 times, most recently from 4178cf9 to 804f8cb Compare August 29, 2024 09:02
@github-actions github-actions bot added the component: dynamodb DynamoDB integration related issue label Aug 29, 2024
@sondemar
Copy link
Contributor Author

#646

Hi @tomazfernandes ,

I have just pushed the following changes:

  1. Added observability for the SQS listener pipeline.
  2. Introduced observability for SqsTemplate.
  3. Created a test for your proposed scenario in this comment.

@sondemar sondemar force-pushed the micrometer-observation-support branch from 804f8cb to bcd72bb Compare August 30, 2024 05:41
@sondemar sondemar force-pushed the micrometer-observation-support branch from bcd72bb to e689285 Compare October 1, 2024 09:19
@github-actions github-actions bot removed the component: dynamodb DynamoDB integration related issue label Oct 1, 2024
@sondemar
Copy link
Contributor Author

Hello @tomazfernandes, can I do anything to help make the review easier?

@AnakinPt
Copy link

I have just one question. This will send the trace when we send sns messages and we have SQS subscribing the SNS?

@sondemar
Copy link
Contributor Author

I have just one question. This will send the trace when we send sns messages and we have SQS subscribing the SNS?

Hi @AnakinPt , According to this comment, the SnsTemplate is not included in this PR. As I mentioned previously in this earlier comment:

"Initially, I considered including support for SQS/SNS templates in a separate ticket, but including SqsTemplate with this ticket is also fine."

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: sqs SQS integration related issue status: team-discussion Team has to figure out how to proceed type: documentation Documentation or Samples related issue type: feature Integration with a new AWS service or bigger change in existing integration
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for Micrometer's Observation API
4 participants