search

rabbitmq / rabbitmq-dotnet-client

RabbitMQ .NET client for .NET Standard 2.0+ and .NET 4.6.2+
https://www.rabbitmq.com/dotnet.html
Other
2.05k stars 575 forks source link

Adding proper OpenTelemetry integration via. registration helpers and better context propagation #1528

Closed stebet closed 1 month ago

stebet commented 3 months ago

Proposed Changes

Types of Changes

Checklist

Further Comments

Would love to get some input from @lmolkova and @martinjt as well :)

stebet commented 3 months ago

@lukebakken and @michaelklishin This PR is not quite ready yet, want to do a little cleanup first to make sure everything works as intended :)

lukebakken commented 3 months ago

@stebet we can leave this PR as a Draft until it's ready.

stebet commented 3 months ago

@lukebakken I'm thinking I might just move the OTel integration tests to the SequentialIntegration test project. They are pretty much doing a very similar thing to the ActivitySource integration tests and just referencing an extra project (not drastically different like the OAuth2 tests). Does that make sense to not convolute the test projects and pipelines?

danielmarbach commented 3 months ago

@stebet thanks for the ping. Currently on vacation and after that pretty occupied with some other things. I'm pinging @lailabougria for a review after her vacation. She has lots of knowledge in this area and it quite likely the better reviewer for this type of work anyway

stebet commented 3 months ago

@lukebakken I think this PR is ready for a proper review now.

Thanks for the feedback @lmolkova and @martinjt, feel free to provide additional comments and feedback if you have time to spare :)

CCing @davidfowl and @jamesnk in case they wants to take a look as well from an Aspire perspective.

stebet commented 3 months ago

This PR should also help with #1478

stebet commented 3 months ago

Think I've resolved most of the comments @lukebakken :)

lailabougria commented 2 months ago

Thanks, @danielmarbach, for the ping!

Regarding @martinjt's comment:

I'm honestly not sure whether the separation of allowing people to turn on/off subscriber or publisher. Is that actually a usecase?

I believe splitting the ActivitySources is ok even though it may not really be necessary eventually; having them split does provide more flexibility and possibly avoids breaking changes down the road.

However, what stood out to me is that the user can't easily benefit from those separate ActivitySources with the proposed instrumentation API, as there's no way to configure them separately and the API adds both sources by default. Assuming users will use the instrumentation API AddRabbitMQInstrumentation, instead of AddSource, they can't opt-in to individual activity sources that way, so they'd have to purposefully use AddSource instead of the instrumentation API, which also means the other code in that method won't be invoked (meaning the baggage propagation issue is still at play, and the propagators aren't set automatically, as @stebet explained in this comment).

That may be something to consider in the design of the instrumentation APIs if the decision is to keep the ActivitySources split.

stebet commented 2 months ago

Thanks, @danielmarbach, for the ping!

Regarding @martinjt's comment:

I'm honestly not sure whether the separation of allowing people to turn on/off subscriber or publisher. Is that actually a usecase?

I believe splitting the ActivitySources is ok even though it may not really be necessary eventually; having them split does provide more flexibility and possibly avoids breaking changes down the road.

However, what stood out to me is that the user can't easily benefit from those separate ActivitySources with the proposed instrumentation API, as there's no way to configure them separately and the API adds both sources by default. Assuming users will use the instrumentation API AddRabbitMQInstrumentation, instead of AddSource, they can't opt-in to individual activity sources that way, so they'd have to purposefully use AddSource instead of the instrumentation API, which also means the other code in that method won't be invoked (meaning the baggage propagation issue is still at play, and the propagators aren't set automatically, as @stebet explained in this comment).

That may be something to consider in the design of the instrumentation APIs if the decision is to keep the ActivitySources split.

Thanks for the input @lailabougria! :)

We discussed this exact thing here: https://github.com/rabbitmq/rabbitmq-dotnet-client/pull/1528#discussion_r1548793930

This PR actually had that configuration at the start but was removed after the discussion.

The consensus seemed to be that it didn't really make sense from the end-user perspective. I'd love more knowledgeable people (from an end-users view) to chime in so we can finish this. Back to you @lmolkova, @cijothomas and @martinjt :)

lukebakken commented 1 month ago

@stebet I merged main into your branch to resolve conflicts, but I had to make this change, which I'm guessing may not be correct:

https://github.com/rabbitmq/rabbitmq-dotnet-client/pull/1528/files#diff-f6d8753c982a8409794d13d5a9263aea70541146d5f4255c86e4e5150f6b9a1dR1072-R1075

Merging #1233 will probably address the above, however.

lukebakken commented 1 month ago

Just FYI for everyone who has commented and contributed here, I would like to release version 7 this month (May 2024).

lukebakken commented 1 month ago

Hi everyone -

In the interest of shipping v7 of this library this month, I am going to merge this PR as-is today or tomorrow. I am going to release RC1 of version 7 by the end-of-day tomorrow, June 4th, 2024.

I am assuming the work here is complete. As I've said in other OpenTelemetry-related PRs, I'm not very familiar with OTel and rely on others' expertise.