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

Add exchange/routingKey to RabbitMessageSenderContext #2816

Merged
merged 4 commits into from
Sep 16, 2024
Merged

Add exchange/routingKey to RabbitMessageSenderContext #2816

merged 4 commits into from
Sep 16, 2024

Conversation

ngocnhan-tran1996
Copy link
Contributor

I see #2814 and I think RabbitMessageSenderContext.java should have field exchange and routingKey.

But I think it's better if we use MessageProperties#setReceivedExchange and MessageProperties#setReceivedRoutingKey instead.

@artembilan
Copy link
Member

The setReceivedExchange and setReceivedRoutingKey are for consumer side.
Since there is no options in the org.springframework.amqp.core.Message fro destination properties, we don't have have unless provide them separately.

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.

Also add your name to the @author list of all the affected classes.
Don't you think that we need to cover the change somehow in the tests?
Or that one is going to be follow up as a fix for the #2814 ?

Thanks

@ngocnhan-tran1996
Copy link
Contributor Author

This one is going to be follow up as a fix for the #2814

return super.getLowCardinalityKeyValues(context).and("foo", "bar");
return super.getLowCardinalityKeyValues(context).and("foo", "bar")
.and("messaging.destination.name", context.getExchange())
.and("messaging.rabbitmq.destination.routing_key", context.getRoutingKey());
Copy link
Member

Choose a reason for hiding this comment

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

I guess this will go to TemplateLowCardinalityTags in the next contribution.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think now it's more easier to determine exchange or routing key for doing traces/metrics without splitting / in RabbitMessageSenderContext.

@artembilan artembilan added this to the 3.2.0-M3 milestone Sep 16, 2024
@artembilan artembilan merged commit 7d1e696 into spring-projects:main Sep 16, 2024
3 checks passed
@vmeunier
Copy link
Contributor

@artembilan
I don't think my issue is still worth anything now since the tags were added here. I was actually trying to change this now and ended up checking at the code "it's already done ?! Did I smoke anything ?"

@ngocnhan-tran1996
Copy link
Contributor Author

@vmeunier

I follow your issue and added field exchange/routingkey. I think it will be easier for you to implement metrics/traces, and IMO it's not done yet.

@vmeunier
Copy link
Contributor

@ngocnhan-tran1996 ok you're right, I don't know why you needed to add those informations, it confused me since I knew I had that stuff to change :)

@artembilan
Copy link
Member

I believe the original issue talks about adding something like:

					.and("messaging.destination.name", context.getExchange())
					.and("messaging.rabbitmq.destination.routing_key", context.getRoutingKey());

into out-of-the-box convention.
That one is not done yet, so keeping it opened until respective contribution.
Thanks

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

Successfully merging this pull request may close these issues.

3 participants