search

bbc / sqs-consumer

Build Amazon Simple Queue Service (SQS) based applications without the boilerplate
https://bbc.github.io/sqs-consumer/
Other
1.71k stars 330 forks source link

feat: add pre and post receive message callbacks #426

Closed karolsojko closed 8 months ago

karolsojko commented 9 months ago

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:

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:

Checklist:

github-actions[bot] commented 9 months ago

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

codeclimate[bot] commented 9 months ago

Code Climate has analyzed commit 95c12dce 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 commented 9 months ago

Hey, thanks for the PR!

If you could just follow the instructions here first that'd be great: https://github.com/bbc/sqs-consumer/pull/426#issuecomment-1759418312

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 commented 9 months ago

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

karolsojko commented 9 months ago

recheck

karolsojko commented 9 months ago

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

nicholasgriffintn commented 9 months ago

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 commented 9 months ago

@nicholasgriffintn any chances for a merge :) ?

nicholasgriffintn commented 9 months ago

@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 commented 8 months ago

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.