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

feat: add pre and post receive message callbacks #426

Merged
merged 2 commits into from
Oct 16, 2023

Conversation

karolsojko
Copy link
Contributor

Resolves #17

Description:
This adds an option to hook your code just before and right after the SQS Client sends the ReceiveMessageCommand.

Type of change:

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Why is this change required?:

This is a change that is required if you'd like to use OpenTelemetry in an organised way with their official js instrumentation for AWS SDK.

The way open telemetry works is that it creates Traces for what's going on in a distributed system. Think of them as a request or single SQS message processing. Those traces are then broken down into Spans later on for things like db communication, http request etc. Usually the top level span you'd want to create in a trace is one that is representing the consumer service. And that is where the problem kicks in. The way that OpenTelemetry AWS SDK Instrumentation works is that it patches the module exports of the SQS Client

By doing so it is able to wrap the SQS communication of ReceiveMessageCommand into a Segment of its own. So this way when you visualise your telemetry you are stuck with having the SQS queue as a service instead of the actual consumer of the SQS queue. Hooking up on the handleMessage function in order to wrap the top level segment is a bit too late since the ReceiveMessageCommand has been already executed at this point.

Code changes:

  • Added optional callbacks to the ConsumerOptions

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@karolsojko karolsojko requested review from a team as code owners October 12, 2023 10:48
@github-actions
Copy link

github-actions bot commented Oct 12, 2023

CLA Assistant Lite bot CLA CHECK All Contributors have signed the CLA

@codeclimate
Copy link

codeclimate bot commented Oct 12, 2023

Code Climate has analyzed commit 95c12dc and detected 1 issue on this pull request.

Here's the issue category breakdown:

Category Count
Complexity 1

The test coverage on the diff in this pull request is 100.0% (80% is the threshold).

This pull request will bring the total coverage in the repository to 97.0% (0.0% change).

View more on Code Climate.

@nicholasgriffintn
Copy link
Member

Hey, thanks for the PR!

If you could just follow the instructions here first that'd be great: #426 (comment)

After that, you just need to run the command to format your code (that's why the tests are currently erroring). There is a complexity issue from CodeClimate but we can pick this up as later clean up, I don't think it's entirely too bad.

Aside from that, do you have an example of where you're using OpenTelemetry with SQS Consumer? I'd be interested in how this is being used, not had someone mention that they're using it before.

@karolsojko
Copy link
Contributor Author

I have read the CLA Document and I hereby sign the CLA

@karolsojko
Copy link
Contributor Author

recheck

@karolsojko
Copy link
Contributor Author

Aside from that, do you have an example of where you're using OpenTelemetry with SQS Consumer? I'd be interested in how this is being used, not had someone mention that they're using it before.

Sure :)

So here's where I have the Consumer wrapped: https://github.com/standardnotes/server/blob/1632c83217d0f466e8686e13829fd1ce1881a252/packages/domain-events-infra/src/Infra/SQS/SQSDomainEventSubscriberFactory.ts

Here is where I have the handleMessage defined (also encapsulate it with a span using OpenTelemetry): https://github.com/standardnotes/server/blob/1632c83217d0f466e8686e13829fd1ce1881a252/packages/domain-events-infra/src/Infra/SQS/SQSOpenTelemetryEventMessageHandler.ts

Copy link
Member

@nicholasgriffintn nicholasgriffintn left a comment

Choose a reason for hiding this comment

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

This looks like it does the job to me, thanks!

@nicholasgriffintn
Copy link
Member

Aside from that, do you have an example of where you're using OpenTelemetry with SQS Consumer? I'd be interested in how this is being used, not had someone mention that they're using it before.

Sure :)

Thanks, that's super helpful for us to understand use cases.

@karolsojko
Copy link
Contributor Author

@nicholasgriffintn any chances for a merge :) ?

@nicholasgriffintn
Copy link
Member

@nicholasgriffintn any chances for a merge :) ?

Hey, yeah it'll be merged just before we're ready to create a release, which will be Monday if I don't get to it any time sooner, leaving it un merged so we don't forget to release it.

@nicholasgriffintn nicholasgriffintn merged commit 63d146a into bbc:main Oct 16, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Oct 16, 2023
@nicholasgriffintn
Copy link
Member

Released as v7.4.0-canary.0, we'll be testing that version and then release it as 7.4.0 once we're happy with the changes.

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

Successfully merging this pull request may close these issues.

Hook before starting processing messages
2 participants