-
Notifications
You must be signed in to change notification settings - Fork 584
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 OpenTelemetry support via ActivitySource #1261
Conversation
It would be great to get some feedback from @cijothomas, @MikeGoldsmith and @martinjt if they have time. I think I've populated the most important tags that are easy to get, but in case I'm missing some, it'd be great to get feedback. |
This should hopefully be ready now. Feel free to give it another pass @cijothomas if you have time :) @bollhals might find this interesting as well. |
Would you mind running some tests on this (with and without tracing enabled)? I suspect some boxing occurring even in the disabled case. One question I had was do we need a separate option to enable it? E.g. my business has tracing enabled, but we'd might not want to know this level of detail from RabbitMQ. |
Yeah, will do. Although even when tracing is enabled, if the activity isn't sampled, it'll still skip A LOT of the processing since it checks first if the Activity is requesting all data. B.t.w, overhead is expected, but the APIs are very fast and used for both runtime stuff (SQL client, HTTP client) as well as as third party libraries like StackExchange.Redis so i.m.o the overhead when tracing is enabled is acceptable and known. That's why Sampling is a built-in feature for Activities.
Yes. For example if you are using the OpenTelemetry libraries you'll need to do something like this: builder.Services.AddOpenTelemetryTracing(builder =>
{
builder
.Configure((IServiceProvider serviceProvider, TracerProviderBuilder traceBuilder) =>
{
traceBuilder
.SetResourceBuilder(ResourceBuilder.CreateDefault().AddService("test-app", serviceVersion: "1.0.0"))
.AddSource("RabbitMQ.Client") // Enable tracing for the RabbitMQ.Client library. Not setting this causes no activities to be created within the library.
.AddAspNetCoreInstrumentation(options => options.RecordException = true)
.... // more initialization stuff here
;
});
}); |
projects/RabbitMQ.Client/client/events/AsyncEventingBasicConsumer.cs
Outdated
Show resolved
Hide resolved
Yes, sure it's expected if you enable such a feature, but there are still things we could optimize for to avoid allocations. But this can also wait for later. But we should look at overhead of the two other options of no tracing enabled and tracing enabled but not for RabbitMQ.
Ah yes, forgot about it, as I only had to do it once for our project :D Thanks for reminding me. |
@bollhals ActivitySource build with OpenTelemetry initialized but not listening for |
Hi @stebet, thanks for making this PR. Do you think we could have it in the next coming months? |
52b4552
to
2461db8
Compare
Rebased this on the latest master and removed the need for |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There were a lot of changes related to tag-naming for messaging. Still, it is not stable version, but it reflects current state.
Updated to be in line with current spec. Thanks @Kielek, would love it if you could verify that I got things correcly :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One obvious typo.
I am not an subject expert for messaging system so please double check the second one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Traces` attributes LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks.
Would you add examples/docs as a follow up? The name of the ActivitySource
used must be documented so OTel consumers can enable this instrumentation.
Also - please call out that this relies on experimental semantic convention and is subject to change, so users are not caught with surprise if the conventions change before they stabilize.
If people need this feature soon, a user has found the following workaround that uses AMQP headers: https://github.com/open-telemetry/opentelemetry-dotnet/tree/main/examples/MicroserviceExample |
@cijothomas I personally know next to nothing about OpenTelemetry and the changes in this PR, so someone who is more familiar will have to address your concerns:
|
@stebet I'm merging the latest |
86c6e87
to
e40f2ac
Compare
@stebet I have completed rebasing this PR on |
I'll take a look in a bit. |
e40f2ac
to
d34160b
Compare
@stebet everything seems to be fine, except that the Of course it works every time on my machine. |
I'll take a look now and see if I see anything obvious |
@lukebakken Moved the Activity tests over to the sequential tests since they are manipulating static variables. |
Glad to see this making progress! |
… enable OpenTelemetry scenarios. Linking existing context for publish if one exists dotnet format Adding standard tags without allocating. Updating code after comments Moving ActivitySource tests to Integration Making sure TaskCompletionSources execute asynchronously Moving activity source tests to sequential integration
0535eda
to
58266fc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@stebet if you have time to address these comments that would be great:
- Add OpenTelemetry support via ActivitySource #1261 (review)
- Add OpenTelemetry support via ActivitySource #1261 (comment) @cijothomas - if you can address this comment of yours that would be great.
- Documentation/examples is handled in another repo AFAIK - @stebet are you referring to the repo that is used to create this web site?
Will do.
Yes. |
@cijothomas I would appreciate example code and doc text that I can use to update the web site. Thanks. |
Excellent! I think we'd like to get this backported to the older version. Before we do that though, is it possible to get a preview build that we can ship with an aspire preview so we can get some mileage on it? |
Proposed Changes
This is the basic implementation to add OpenTelemetry support to the .NET RabbitMQ Client. This implements an ActivitySource that creates and propagates Activities for BasicPublish (send), BasicDeliver (receive) and when consumers receive messages (process). This enables distributed tracing scenarios such as showing traces. Below is a screenshot from Honeycomb.io to show what happens when tracing is enabled.
Types of Changes
What types of changes does your code introduce to this project?
Put an
x
in the boxes that applyChecklist
CONTRIBUTING.md
documentFurther Comments
This should have minimal overhead when tracing is not enabled since no Activity generation occurs and no tags need to be populated.
Currently, to propagate the trace and span ids to correlate events, I'm taking advantage of the
BasicProperties.Headers
to send and parse the information needed to correlate individual traces.Here is a screenshot from Honeycomb of what this can look like: