Adding proper OpenTelemetry integration via. registration helpers and better context propagation #1528
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| # RabbitMQ .NET Client - OAuth2 |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,64 @@ | ||
| <Project Sdk="Microsoft.NET.Sdk"> | ||
|
|
||
| <PropertyGroup> | ||
| <TargetFrameworks>net6.0;netstandard2.0</TargetFrameworks> | ||
| <NoWarn>$(NoWarn);CS1591</NoWarn> | ||
| <TreatWarningsAsErrors>true</TreatWarningsAsErrors> | ||
| <AssemblyTitle>RabbitMQ OpenTelemetry Integration Package for .NET</AssemblyTitle> | ||
| <Authors>VMware</Authors> | ||
| <Company>VMware, Inc. or its affiliates.</Company> | ||
| <Copyright>Copyright © 2007-2023 VMware, Inc. or its affiliates.</Copyright> | ||
| <Description>The RabbitMQ OAuth2 Client Library for .NET enables OAuth2 token refresh for RabbitMQ.Client</Description> | ||
| <GenerateDocumentationFile>true</GenerateDocumentationFile> | ||
| <PackageIcon>icon.png</PackageIcon> | ||
| <PackageLicenseExpression>Apache-2.0 OR MPL-2.0</PackageLicenseExpression> | ||
| <PackageProjectUrl>https://www.rabbitmq.com/dotnet.html</PackageProjectUrl> | ||
| <PackageTags>rabbitmq, amqp, oauth2</PackageTags> | ||
| <Product>RabbitMQ</Product> | ||
| <PublishRepositoryUrl>true</PublishRepositoryUrl> | ||
| <RepositoryUrl>https://github.com/rabbitmq/rabbitmq-dotnet-client.git</RepositoryUrl> | ||
| <IncludeSymbols>true</IncludeSymbols> | ||
| <SymbolPackageFormat>snupkg</SymbolPackageFormat> | ||
| <AssemblyOriginatorKeyFile>../rabbit.snk</AssemblyOriginatorKeyFile> | ||
| <SignAssembly>true</SignAssembly> | ||
| <MinVerTagPrefix>otel-</MinVerTagPrefix> | ||
| <MinVerVerbosity>minimal</MinVerVerbosity> | ||
| <GeneratePackageOnBuild>true</GeneratePackageOnBuild> | ||
| <PackageOutputPath>../../packages</PackageOutputPath> | ||
| <PackageReadmeFile>README.md</PackageReadmeFile> | ||
| <LangVersion>7.3</LangVersion> | ||
lukebakken marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| </PropertyGroup> | ||
|
|
||
| <PropertyGroup Condition="'$(Configuration)' == 'Release' And '$(CI)' == 'true'"> | ||
| <ContinuousIntegrationBuild>true</ContinuousIntegrationBuild> | ||
| <Deterministic>true</Deterministic> | ||
| <EmbedUntrackedSources>true</EmbedUntrackedSources> | ||
| </PropertyGroup> | ||
|
|
||
| <ItemGroup Condition="'$(Configuration)' == 'Release' and '$(SourceRoot)' == ''"> | ||
| <SourceRoot Include="$(MSBuildThisFileDirectory)/" /> | ||
| </ItemGroup> | ||
|
|
||
| <ItemGroup> | ||
| <None Remove="icon.png" /> | ||
| <Content Include="icon.png" PackagePath="" /> | ||
| <None Include="README.md" Pack="true" PackagePath="/" /> | ||
| <InternalsVisibleTo Include="Unit" /> | ||
| <InternalsVisibleTo Include="Benchmarks" /> | ||
| </ItemGroup> | ||
|
|
||
| <ItemGroup> | ||
| <PackageReference Include="Microsoft.CodeAnalysis.PublicApiAnalyzers" Version="3.3.4" PrivateAssets="all" /> | ||
| <PackageReference Include="Microsoft.SourceLink.GitHub" Version="8.0.0" PrivateAssets="all" /> | ||
| <PackageReference Include="MinVer" Version="5.0.0" PrivateAssets="all" /> | ||
| <PackageReference Include="OpenTelemetry.Api" Version="1.7.0" /> | ||
| </ItemGroup> | ||
|
|
||
| <ItemGroup Condition="$(TargetFramework) == 'netstandard2.0'"> | ||
| </ItemGroup> | ||
|
|
||
| <ItemGroup> | ||
| <ProjectReference Include="../RabbitMQ.Client/RabbitMQ.Client.csproj" /> | ||
| </ItemGroup> | ||
|
|
||
| </Project> | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,10 @@ | ||
| namespace RabbitMQ.Client.OpenTelemetry | ||
| { | ||
| public class RabbitMQOpenTelemetryConfiguration | ||
| { | ||
| public bool PropagateBaggage { get; set; } = true; | ||
| public bool UseRoutingKeyAsOperationName { get; set; } = true; | ||
| public bool IncludePublishers { get; set; } = true; | ||
| public bool IncludeSubscribers { get; set; } = true; | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,85 @@ | ||
| using System; | ||
| using System.Collections.Generic; | ||
| using System.Diagnostics; | ||
| using System.Linq; | ||
| using System.Text; | ||
| using OpenTelemetry.Context.Propagation; | ||
| using RabbitMQ.Client; | ||
| using RabbitMQ.Client.OpenTelemetry; | ||
|
|
||
| namespace OpenTelemetry.Trace | ||
| { | ||
| public static class OpenTelemetryExtensions | ||
| { | ||
| internal static TextMapPropagator s_propagator = Propagators.DefaultTextMapPropagator; | ||
|
|
||
| public static TracerProviderBuilder AddRabbitMQ(this TracerProviderBuilder builder, | ||
| RabbitMQOpenTelemetryConfiguration configuration) | ||
| { | ||
| if (configuration.PropagateBaggage) | ||
|
There was a problem hiding this comment. any reason not to allow user to provide a preconfigured propagator instance? There was a problem hiding this comment. No reason bu my lack of knowledge :). Are you then referring to these as alternatives to the tracestate propagator? Are there examples for me to look at? There was a problem hiding this comment. I mean you're forcing a specific propagator - a I believe you should just use the If I understand the intent correctly you want to users to be able to disable baggage propagation - any specific reason? There was a problem hiding this comment.
Ahh gotcha! Saw that the DefaultTextMapPropagator was a Noop so I thought this was the way. I'll change it :)
Nope. I'll remove it. Does it make sense to then have a composite propagator with the default textmap propagator and the baggage propagator or do users normally configure the baggage propagator themselves? There was a problem hiding this comment. The otel sdk should set the default one to the composite. Not completely sure what sets |
||
| { | ||
| s_propagator = new CompositeTextMapPropagator(new TextMapPropagator[] | ||
| { | ||
| new TraceContextPropagator(), new BaggagePropagator() | ||
| }); | ||
| } | ||
| else | ||
| { | ||
| s_propagator = new TraceContextPropagator(); | ||
| } | ||
|
|
||
| RabbitMQActivitySource.UseRoutingKeyAsOperationName = configuration.UseRoutingKeyAsOperationName; | ||
|
There was a problem hiding this comment. do you feel it's something we should document better /provide more clear guidance on in the messaging semconv? There was a problem hiding this comment. Might be a good idea if people are running into high-cardinality issues with their messaging span names. There was a problem hiding this comment. Is Operation the right name here? Since this is focused on OpenTelemetry, I'm wondering if just "SpanName" is a better wording. There was a problem hiding this comment. OTel config: There was a problem hiding this comment.
the span name cardinality requirements are soft ( so if cardinality is the only problem, I'd not make span name configurable (worst case users can rename spans with a processor). So again, no strong opinions |
||
| RabbitMQActivitySource.ContextExtractor = OpenTelemetryContextExtractor; | ||
| RabbitMQActivitySource.ContextInjector = OpenTelemetryContextInjector; | ||
|
Comment on lines
+16
to
+17
There was a problem hiding this comment. Why we need this part? Can it be set by default in main package? I would expect that this method will only call Alternative options is to have here the second method: public static TracerProviderBuilder AddRabbitMQInstrumentation(this TracerProviderBuilder builder, Action<RabbitMQSpecificOptions>? configure);where you can configure other methods externally. There was a problem hiding this comment. OpenTelemetryContextInjector is OTel specific for Baggage handling, that's pretty much the only reason for the package, because we don't want to pull OpenTelemetry.API dependencies into the main library. |
||
|
|
||
| if (configuration.IncludeSubscribers) | ||
|
There was a problem hiding this comment. I don't have any strong opinion here, but want to challenge the need for this flag (and I guess my broader question is when would users want to enable rabbitmq via an instrumentation library if all they really need to do is call There was a problem hiding this comment.
The main reason for the library was to simplify the OTel configuration and setup for users (with the extension method) and to work around the current issue that since OTel and Activity Baggage propagation is different we do not have to make the Also it takes care of configuring the custom propagators to use the OTel propagators instead of the default Activity propagators. I also just changed the defaults in the latest commits to have both publishers and subscriber event sources enabled, so users can just call There was a problem hiding this 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 can understand adding it here if that's a usecase seeing as this is for simplification. My question is, should you allow people to just do one or the other? There was a problem hiding this comment.
No idea, but I think it was you and @lmolkova that suggested I separated the ActivitySources in the earlier OTel pr: https://github.com/rabbitmq/rabbitmq-dotnet-client/pull/1261/files#r1392469382 I have no preference either way really There was a problem hiding this comment. Having different names is more future-proof, but also it costs nothing since you can enable sources with wildcard. I didn't try to push you to remove the instrumentation library, was just trying to understand what'd be the better case for it. The There was a problem hiding this comment.
No worries. I didn't take it as such :)
Here's an old issue that relates to it (CC @cijothomas): open-telemetry/opentelemetry-dotnet#1842 There was a problem hiding this comment. The Activity baggage vs OTel Baggage is still an unresolved problem! It'll eventually be fixed, though we don't have specifics or timeline. (its not likely coming in this year, there is nothing tracking it in .NET 9 release timeframe.) |
||
| { | ||
| builder.AddSource(RabbitMQActivitySource.SubscriberSourceName); | ||
| } | ||
|
|
||
| if (configuration.IncludePublishers) | ||
| { | ||
| builder.AddSource(RabbitMQActivitySource.PublisherSourceName); | ||
| } | ||
|
|
||
| return builder; | ||
| } | ||
|
|
||
| private static ActivityContext OpenTelemetryContextExtractor(IReadOnlyBasicProperties props) | ||
| { | ||
| // Extract the PropagationContext of the upstream parent from the message headers. | ||
| var parentContext = s_propagator.Extract(default, props.Headers, OpenTelemetryContextGetter); | ||
| Baggage.Current = parentContext.Baggage; | ||
| return parentContext.ActivityContext; | ||
| } | ||
|
|
||
| private static IEnumerable<string> OpenTelemetryContextGetter(IDictionary<string, object> carrier, string key) | ||
| { | ||
| try | ||
| { | ||
| if (carrier.TryGetValue(key, out object value)) | ||
| { | ||
| byte[] bytes = value as byte[]; | ||
| return new[] { Encoding.UTF8.GetString(bytes) }; | ||
| } | ||
| } | ||
| catch (Exception) | ||
| { | ||
| //this.logger.LogError(ex, "Failed to extract trace context."); | ||
| } | ||
|
|
||
| return Enumerable.Empty<string>(); | ||
| } | ||
|
|
||
| private static void OpenTelemetryContextInjector(Activity activity, IDictionary<string, object> props) | ||
| { | ||
| // Inject the current Activity's context into the message headers. | ||
| s_propagator.Inject(new PropagationContext(activity.Context, Baggage.Current), props, OpenTelemetryContextSetter); | ||
| } | ||
|
|
||
| private static void OpenTelemetryContextSetter(IDictionary<string, object> carrier, string key, string value) | ||
| { | ||
| carrier[key] = Encoding.UTF8.GetBytes(value); | ||
| } | ||
| } | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.