feat: Add the ability to use span links when consuming a message amqp plugin #1972
Conversation
|
|
There was a problem hiding this comment.
I assume this was made a configuration because it is a breaking change. We will probably try to go to this as the default behavior in a future version when the messaging semconv stabilizes. For now, I think this is a decent compromise.
Can you please add tests for the new behavior?
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1972 +/- ##
==========================================
- Coverage 90.97% 90.42% -0.56%
==========================================
Files 146 148 +2
Lines 7492 7333 -159
Branches 1502 1524 +22
==========================================
- Hits 6816 6631 -185
- Misses 676 702 +26
|
I added in tests for the new option. And yeah agree with you in the future, likely we will switch over the default. This gives us an easy way to do that as well. Let me know if you need anything else! I ran the linter and there are quite a few warnings that for code I did not touch. If you need me to correct those, let me know. |
Warnings are typically fine, especially if they're on code you didn't introduce. There are many hundreds across the codebase and requiring them to be fixed is a losing battle at this point. Mostly the linter just makes the code style consistent enough to make it easier to review. |
Perfect! the code is ready to be reviewed then. |
|
This PR touches on 2 things:
I wonder if they are mutually exclusive. Always recording a link to the producer makes sense to me, and is not a breaking change. regarding the parent-child relationship - I can see how some users would want it one way and others will want it the other way (regardless of the default value). perhaps the new config option can address this property instead? something like |
| `${queue} process`, | ||
| { | ||
| let span: Span; | ||
| if (self._config.useLinksForConsume) { |
There was a problem hiding this comment.
the 2 cases in the if-else contains some duplication.
WDYT about preparing the links array only once (with 0 or 1 elements) and the parent based on the instrumentation configuration, then just invoking self.tracer.startSpan once with these values? or at least calculate the attributes once, outside the if-else
There was a problem hiding this comment.
Would a null parent be fine in this instance? Because with the config option, the parentcontext should not exists. If that's fine, then I can make those changes.
There was a problem hiding this comment.
undefined is fine. It's the default anyway
There was a problem hiding this comment.
I'd do something like this
const links: Link[] = [];
if (self._config.useLinksForConsume) {
const parentSpanContext = trace.getSpan(parentContext)?.spanContext();
if (parentSpanContext) {
links.push({ context: parentSpanContext });
}
}
const span = self.tracer.startSpan(`${queue} process`, {
kind: SpanKind.CONSUMER,
attributes: {
...channel?.connection?.[CONNECTION_ATTRIBUTES],
[SemanticAttributes.MESSAGING_DESTINATION]: exchange,
[SemanticAttributes.MESSAGING_DESTINATION_KIND]:
MessagingDestinationKindValues.TOPIC,
[SemanticAttributes.MESSAGING_RABBITMQ_ROUTING_KEY]:
msg.fields?.routingKey,
[SemanticAttributes.MESSAGING_OPERATION]:
MessagingOperationValues.PROCESS,
[SemanticAttributes.MESSAGING_MESSAGE_ID]:
msg?.properties.messageId,
[SemanticAttributes.MESSAGING_CONVERSATION_ID]:
msg?.properties.correlationId,
},
links,
},
self._config.useLinksForConsume ? undefined : parentContext,
);
There was a problem hiding this comment.
@McSick looks like you ended up making the change for this, albeit a bit more verbose. I think it achieves the same thing though. As a nit I tend to lean toward the more succinct version but non blocking.
There was a problem hiding this comment.
I had to change it a little as parentContext was used deeper in the code so I wanted to explicitly make it undefined in this instance so the trace was not continued.
I don't think they're exclusive. If you remove the parent-child link you lose all relationship without the link. If you add the link, it is a bit overlapping with the parent-child relationship and potentially confusing. Because this is opt-in, I think it is probably fine. edit: Also, this is just implementing the currently specified behavior. It could be argued we don't have a choice. |
| `${queue} process`, | ||
| { | ||
| let span: Span; | ||
| if (self._config.useLinksForConsume) { |
There was a problem hiding this comment.
I'd do something like this
const links: Link[] = [];
if (self._config.useLinksForConsume) {
const parentSpanContext = trace.getSpan(parentContext)?.spanContext();
if (parentSpanContext) {
links.push({ context: parentSpanContext });
}
}
const span = self.tracer.startSpan(`${queue} process`, {
kind: SpanKind.CONSUMER,
attributes: {
...channel?.connection?.[CONNECTION_ATTRIBUTES],
[SemanticAttributes.MESSAGING_DESTINATION]: exchange,
[SemanticAttributes.MESSAGING_DESTINATION_KIND]:
MessagingDestinationKindValues.TOPIC,
[SemanticAttributes.MESSAGING_RABBITMQ_ROUTING_KEY]:
msg.fields?.routingKey,
[SemanticAttributes.MESSAGING_OPERATION]:
MessagingOperationValues.PROCESS,
[SemanticAttributes.MESSAGING_MESSAGE_ID]:
msg?.properties.messageId,
[SemanticAttributes.MESSAGING_CONVERSATION_ID]:
msg?.properties.correlationId,
},
links,
},
self._config.useLinksForConsume ? undefined : parentContext,
);
|
@McSick Thanks again for working on this. |
|
I do need to finalize it. Ive been pulled into some other things but will try to get it done within the next week. |
| `${queue} process`, | ||
| { | ||
| let span: Span; | ||
| if (self._config.useLinksForConsume) { |
There was a problem hiding this comment.
@McSick looks like you ended up making the change for this, albeit a bit more verbose. I think it achieves the same thing though. As a nit I tend to lean toward the more succinct version but non blocking.
|
@JamieDanielson , went ahead and updated based on your suggestions and updated the readme to resolve the conflict. |
There was a problem hiding this comment.
@McSick Hmm I think something got weird with a merge from a remote branch somewhere along the way here... maybe this commit? I found one section of the README that should be removed, suggestion highlighted. I don't think there is anything in the other files but I need to take another look through.
I went through and did some branch cleanup to check things out and it seems like this README change is the only change (aside from whitespace)... so I committed that and the tests are running. I included the git diff for my branch and yours below. git diffdiff --git a/plugins/node/instrumentation-amqplib/README.md b/plugins/node/instrumentation-amqplib/README.md
index dcfac7fb..dd211d3a 100644
--- a/plugins/node/instrumentation-amqplib/README.md
+++ b/plugins/node/instrumentation-amqplib/README.md
@@ -51,7 +51,7 @@ registerInstrumentations({
amqplib instrumentation has few options available to choose from. You can set the following:
| Options | Type | Description |
-| --------------------------------- | ----------------------------------------- | -------------------------------------------------------------------------------------------------------------------------- |
+| -------------------- | ---------------------------------------------- | ----------------------------------------------------------------------------------- |
| `publishHook` | `AmqplibPublishCustomAttributeFunction` | hook for adding custom attributes before publish message is sent. |
| `publishConfirmHook` | `AmqplibPublishConfirmCustomAttributeFunction` | hook for adding custom attributes after publish message is confirmed by the broker. |
| `consumeHook` | `AmqplibConsumeCustomAttributeFunction` | hook for adding custom attributes before consumer message is processed. |
@@ -77,18 +77,6 @@ By default, consume spans continue the trace where a message was produced. Howev
Default is false
-## Migration From opentelemetry-instrumentation-amqplib
-
-This instrumentation was originally published under the name `"opentelemetry-instrumentation-amqplib"` in [this repo](https://github.com/aspecto-io/opentelemetry-ext-js). Few breaking changes were made during porting to the contrib repo to align with conventions:
-
-### Hook Info
-
-The instrumentation's config `publishHook`, `publishConfirmHook`, `consumeHook` and `consumeEndHook` functions signature changed, so the second function parameter is info object, containing the relevant hook data.
-
-### `moduleVersionAttributeName` config option
-
-The `moduleVersionAttributeName` config option is removed. To add the amqplib package version to spans, use the `moduleVersion` attribute in hook info for `publishHook` and `consumeHook` functions.
-
## Running Tests Locally
To run the tests locally, you need to have a RabbitMQ server running. You can use the following command to start a RabbitMQ server using Docker: |
Thank you @JamieDanielson , sorry for the delay, busy days at work 🥵 |
|
The failed test is for browser and unrelated, looks like a flaky timeout issue. Rerunning just in case |
Which problem is this PR solving?
Short description of the changes