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

Use ProducerContext as active context in Consumer #10

Closed
Steffen911 opened this issue Oct 7, 2024 · 6 comments · Fixed by #11
Closed

Use ProducerContext as active context in Consumer #10

Steffen911 opened this issue Oct 7, 2024 · 6 comments · Fixed by #11

Comments

@Steffen911
Copy link
Contributor

Summary

I would like to request a feature flag that toggles whether the ProducerContext should be reused as the ConsumerContext, i.e. Producer and Consumer would be within the same trace.

Current behaviour

We currently observe that the consumer is linked to the producer, but in our APM tools (Datadog, Jaeger) spans aren't shown in the same burn chart. In our case, it would be desirable to use the same TraceId for Consumer and Producer.

Expected behaviour

Add a feature flag to use the Producer Context as active context when creating the consumer span. My assumption is that this would be an update in https://github.com/appsignal/opentelemetry-instrumentation-bullmq/blob/main/src/instrumentation.ts#L449 to use producerContext instead of currentContext. As this would be a breaking behaviour in certain scenarios, I would toggle that flag to false by default.

@Steffen911
Copy link
Contributor Author

@unflxw Would you have any concerns about this proposal? I'm happy to start a pull request if this has a chance of getting merged.

@unflxw
Copy link
Collaborator

unflxw commented Oct 7, 2024

Hi @Steffen911, thanks for the suggestion!

I am okay with this being implemented with a false-by-default configuration flag, as this is a common behaviour in other OpenTelemetry background job instrumentations. Please feel free to send a PR our way.

Do note, however, that the OpenTelemetry Semantic Conventions for Messaging Spans say that links should be used:

A single “Process” or “Receive” span can account for a single message, for a batch of messages, or for no message at all (if it is signalled that no messages were received). For each message it accounts for, the “Process” or “Receive” span SHOULD link to the message’s creation context.

I think the reason for this is to support batch message processing. BullMQ Pro does have batch message processing functionality, but this instrumentation currently does not support it. With batch message processing, the same process spans would need to have multiple parents. Anyway, this is not an issue at the moment.

Finally, do note that it is possible to see span links in Datadog, though not as part of the same trace.

My assumption is that this would be an update in https://github.com/appsignal/opentelemetry-instrumentation-bullmq/blob/main/src/instrumentation.ts#L449 to use producerContext instead of currentContext.

Yes, and to not create a link using producerContext here: https://github.com/appsignal/opentelemetry-instrumentation-bullmq/blob/main/src/instrumentation.ts#L479-L482

(If they're parent-child, I don't think they should also be linked)

@Steffen911
Copy link
Contributor Author

Thank you for the feedback! I created #11 which should cover this requirement.

@unflxw unflxw closed this as completed in #11 Oct 8, 2024
@Steffen911
Copy link
Contributor Author

@unflxw Thank you for your support here! Could you also do a new release which includes this change?

@unflxw
Copy link
Collaborator

unflxw commented Oct 8, 2024

@Steffen911 Done! @appsignal/[email protected] has been released. Thank you for your contribution!

Do note that I renamed the config option in #12.

@Steffen911
Copy link
Contributor Author

Thank you 🙏

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 a pull request may close this issue.

2 participants