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.

Sign up for GitHub

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

Jump to bottom

RabbitTemplate and RabbitListener add opentelemetry tags #2833

Merged
merged 6 commits into from
Sep 19, 2024
Merged

RabbitTemplate and RabbitListener add opentelemetry tags #2833

merged 6 commits into from
Sep 19, 2024

Conversation

vmeunier
Copy link
Contributor

@vmeunier vmeunier commented Sep 18, 2024 •
edited
Loading

closes #2814

added message.destination.name and message.rabbitmq.destination.routing_key as new tags for RabbitTemplateObservationConvention
added messaging.rabbitmq.message.delivery_tag and message.destination.name to RabbitListenerObservationConvention

@vmeunier vmeunier force-pushed the 2814-add-opentelemetry-tags-rabbitmq-rabbittemplate branch from 17fb186 to a330bc2 Compare September 18, 2024 13:56
@vmeunier vmeunier force-pushed the 2814-add-opentelemetry-tags-rabbitmq-rabbittemplate branch from a330bc2 to 415a763 Compare September 18, 2024 13:57
artembilan
artembilan requested changes Sep 18, 2024
View reviewed changes
Copy link
Member

@artembilan artembilan left a comment

Choose a reason for hiding this comment

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

Just one nit-pick.

When we merge this, tell us, please, if we are OK to contribute similar tags into the RabbitListenerObservation.

Thanks.

.../main/java/org/springframework/amqp/rabbit/support/micrometer/RabbitTemplateObservation.java Outdated Show resolved Hide resolved
.../main/java/org/springframework/amqp/rabbit/support/micrometer/RabbitTemplateObservation.java Outdated Show resolved Hide resolved
@vmeunier
Copy link
Contributor Author

@artembilan I can do the code for Listener as well, I'm not quite sure if "messaging.destination" sounds good when you are a receiver.

@vmeunier vmeunier changed the title RabbitTemplate add opentelemetry tags RabbitTemplate and RabbitListener add opentelemetry tags Sep 19, 2024
artembilan
artembilan requested changes Sep 19, 2024
View reviewed changes
Copy link
Member

@artembilan artembilan left a comment

Choose a reason for hiding this comment

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

Checkstyle violations:

 Error: eckstyle] [ERROR] /home/runner/work/spring-amqp/spring-amqp/spring-rabbit/src/main/java/org/springframework/amqp/rabbit/support/micrometer/RabbitListenerObservation.java:70: First sentence should end with a period. [JavadocStyle]
> Task :spring-rabbit:checkstyleMain
Error: eckstyle] [ERROR] /home/runner/work/spring-amqp/spring-amqp/spring-rabbit/src/main/java/org/springframework/amqp/rabbit/support/micrometer/RabbitListenerObservation.java:82: First sentence should end with a period. [JavadocStyle]
Error: eckstyle] [ERROR] /home/runner/work/spring-amqp/spring-amqp/spring-rabbit/src/main/java/org/springframework/amqp/rabbit/support/micrometer/RabbitTemplateObservation.java:74: First sentence should end with a period. [JavadocStyle]
Error: eckstyle] [ERROR] /home/runner/work/spring-amqp/spring-amqp/spring-rabbit/src/main/java/org/springframework/amqp/rabbit/support/micrometer/RabbitTemplateObservation.java:87: First sentence should end with a period. [JavadocStyle]

Run ./gradlew check locally before pushing to PR.
Thanks

@vmeunier
Copy link
Contributor Author

Sorry, I didn't know IntelliJ needed a plugin for checkstyle... No more issues reported by Gradle

artembilan
artembilan requested changes Sep 19, 2024
View reviewed changes
.../main/java/org/springframework/amqp/rabbit/support/micrometer/RabbitListenerObservation.java Outdated
},

/**
* The exchange the listener is plugged to (empty if default exchange).
Copy link
Member

Choose a reason for hiding this comment

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

The listener deals with queues, not exchanges.
So, I believe here this has to be set into context.getCarrier().getMessageProperties().getConsumerQueue().

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

.../main/java/org/springframework/amqp/rabbit/support/micrometer/RabbitListenerObservation.java Outdated
ROUTING_KEY {
@Override
public String asString() {
return "messaging.rabbitmq.destination.routing_key";
Copy link
Member

Choose a reason for hiding this comment

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

This is totally wrong for consumer side.
See the messaging.rabbitmq.message.delivery_tag instead.
Can be set into a context.getCarrier().getMessageProperties().getDeliveryTag().

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

.../main/java/org/springframework/amqp/rabbit/support/micrometer/RabbitListenerObservation.java Outdated
RabbitListenerObservation.ListenerLowCardinalityTags.EXCHANGE.asString(),
context.getCarrier().getMessageProperties().getReceivedExchange(),
RabbitListenerObservation.ListenerLowCardinalityTags.ROUTING_KEY.asString(),
context.getCarrier().getMessageProperties().getReceivedRoutingKey()
Copy link
Member

Choose a reason for hiding this comment

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

I think for slight optimization the context.getCarrier().getMessageProperties() could be extracted into a local variable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

artembilan
artembilan requested changes Sep 19, 2024
View reviewed changes
.../main/java/org/springframework/amqp/rabbit/support/micrometer/RabbitListenerObservation.java Show resolved Hide resolved
.../main/java/org/springframework/amqp/rabbit/support/micrometer/RabbitListenerObservation.java Show resolved Hide resolved
.../main/java/org/springframework/amqp/rabbit/support/micrometer/RabbitListenerObservation.java Outdated
@@ -63,12 +60,35 @@ public enum ListenerLowCardinalityTags implements KeyName {
* Listener id.
*/
LISTENER_ID {

Copy link
Member

Choose a reason for hiding this comment

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

But this one has to be here for better code readability.

.../main/java/org/springframework/amqp/rabbit/support/micrometer/RabbitListenerObservation.java Show resolved Hide resolved
.../main/java/org/springframework/amqp/rabbit/support/micrometer/RabbitListenerObservation.java Show resolved Hide resolved
@artembilan artembilan merged commit ed0d790 into spring-projects:main Sep 19, 2024
3 checks passed
@artembilan
Copy link
Member

@vmeunier ,

thank you for contribution; looking forward for more!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@artembilan artembilan artembilan approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

OpenTelemetry with RabbitMQ add messaging.rabbitmq.destination.routing_key from the semantic convention
2 participants
@vmeunier @artembilan