open-telemetry
/
opentelemetry-dotnet
mirror of https://github.com/open-telemetry/opentelemetry-dotnet.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

[Http] Fix propagation issues (#3828) 

* Fix propagation issues with http client instrumentation.

* More comments and asserts.

* Fixes.

* Improve tests.

* Test cleanup.

* Simplify things.

* CHANGELOG update.

* Code review.

* Lint.

* Code review.

* Code review.

Co-authored-by: Vishwesh Bankwar <vishweshbankwar@users.noreply.github.com>
This commit is contained in:
Mikel Blanchard 2022-10-28 10:48:24 -07:00 committed by GitHub
parent 1a65aec968
commit 8add3db43a
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
3 changed files with 94 additions and 28 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

14
src/OpenTelemetry.Instrumentation.Http/CHANGELOG.md
View File

@ -2,7 +2,10 @@
## Unreleased
* *Breaking change** The `Enrich` callback option has been removed. For better
* Added back `netstandard2.0` target.
([#3787](https://github.com/open-telemetry/opentelemetry-dotnet/pull/3787))
* **Breaking change**: The `Enrich` callback option has been removed. For better
usability, it has been replaced by three separate options: In case of
`HttpClient` the new options are `EnrichWithHttpRequestMessage`,
`EnrichWithHttpResponseMessage` and `EnrichWithException` and in case of
@ -15,11 +18,12 @@
`HttpClient` and `HttpWebRequest`,`HttpWebResponse` and `Exception` in case of
`HttpWebRequest`. The separate callbacks make it clear what event triggers
them and there is no longer the need to cast the argument to the expected
type.
([#3792](https://github.com/open-telemetry/opentelemetry-dotnet/pull/3792))
type.
([#3792](https://github.com/open-telemetry/opentelemetry-dotnet/pull/3792))
* Added back `netstandard2.0` target.
([#3787](https://github.com/open-telemetry/opentelemetry-dotnet/pull/3787))
* Fixed an issue which prevented custom propagators from being called on .NET 7+
runtimes for non-sampled outgoing `HttpClient` spans.
([#3828](https://github.com/open-telemetry/opentelemetry-dotnet/pull/3828))
## 1.0.0-rc9.8

33
src/OpenTelemetry.Instrumentation.Http/Implementation/HttpHandlerDiagnosticListener.cs
View File

@ -100,11 +100,7 @@ namespace OpenTelemetry.Instrumentation.Http.Implementation
// By this time, samplers have already run and
// activity.IsAllDataRequested populated accordingly.
// For .NET7.0 or higher versions, activity is created using activity source
// However, the framework will fallback to creating activity if the sampler's decision is to drop and there is a active diagnostic listener.
// To prevent processing such activities we first check the source name to confirm if it was created using
// activity source or not.
if (Sdk.SuppressInstrumentation || (IsNet7OrGreater && string.IsNullOrEmpty(activity.Source.Name)))
if (Sdk.SuppressInstrumentation)
{
return;
}
@ -122,6 +118,15 @@ namespace OpenTelemetry.Instrumentation.Http.Implementation
textMapPropagator.Inject(new PropagationContext(activity.Context, Baggage.Current), request, HttpRequestMessageContextPropagation.HeaderValueSetter);
}
// For .NET7.0 or higher versions, activity is created using activity source.
// However the framework will fallback to creating activity if the sampler's decision is to drop and there is a active diagnostic listener.
// To prevent processing such activities we first check the source name to confirm if it was created using
// activity source or not.
if (IsNet7OrGreater && string.IsNullOrEmpty(activity.Source.Name))
{
activity.IsAllDataRequested = false;
}
// enrich Activity from payload only if sampling decision
// is favorable.
if (activity.IsAllDataRequested)
@ -171,15 +176,6 @@ namespace OpenTelemetry.Instrumentation.Http.Implementation
public void OnStopActivity(Activity activity, object payload)
{
// For .NET7.0 or higher versions, activity is created using activity source
// However, the framework will fallback to creating activity if the sampler's decision is to drop and there is a active diagnostic listener.
// To prevent processing such activities we first check the source name to confirm if it was created using
// activity source or not.
if (IsNet7OrGreater && string.IsNullOrEmpty(activity.Source.Name))
{
return;
}
if (activity.IsAllDataRequested)
{
// https://github.com/dotnet/runtime/blob/master/src/libraries/System.Net.Http/src/System/Net/Http/DiagnosticsHandler.cs
@ -229,15 +225,6 @@ namespace OpenTelemetry.Instrumentation.Http.Implementation
public void OnException(Activity activity, object payload)
{
// For .NET7.0 or higher versions, activity is created using activity source
// However, the framework will fallback to creating activity if the sampler's decision is to drop and there is a active diagnostic listener.
// To prevent processing such activities we first check the source name to confirm if it was created using
// activity source or not.
if (IsNet7OrGreater && string.IsNullOrEmpty(activity.Source.Name))
{
return;
}
if (activity.IsAllDataRequested)
{
if (!this.stopExceptionFetcher.TryFetch(payload, out Exception exc) || exc == null)

75
test/OpenTelemetry.Instrumentation.Http.Tests/HttpClientTests.Basic.cs
View File

@ -584,6 +584,81 @@ namespace OpenTelemetry.Instrumentation.Http.Tests
Assert.Empty(exportedItems[0].Events);
}
[Theory]
[InlineData(true, true)]
[InlineData(true, false)]
[InlineData(false, true)]
[InlineData(false, false)]
public async Task CustomPropagatorCalled(bool sample, bool createParentActivity)
{
ActivityContext parentContext = default;
ActivityContext contextFromPropagator = default;
var propagator = new Mock<TextMapPropagator>();
propagator.Setup(m => m.Inject(It.IsAny<PropagationContext>(), It.IsAny<HttpRequestMessage>(), It.IsAny<Action<HttpRequestMessage, string, string>>()))
.Callback<PropagationContext, HttpRequestMessage, Action<HttpRequestMessage, string, string>>((context, carrier, setter) =>
{
contextFromPropagator = context.ActivityContext;
setter(carrier, "custom_traceparent", $"00/{contextFromPropagator.TraceId}/{contextFromPropagator.SpanId}/01");
setter(carrier, "custom_tracestate", contextFromPropagator.TraceState);
});
var previousDefaultTextMapPropagator = Propagators.DefaultTextMapPropagator;
Sdk.SetDefaultTextMapPropagator(propagator.Object);
var exportedItems = new List<Activity>();
using (var traceprovider = Sdk.CreateTracerProviderBuilder()
.AddHttpClientInstrumentation()
.AddInMemoryExporter(exportedItems)
.SetSampler(sample ? new ParentBasedSampler(new AlwaysOnSampler()) : new AlwaysOffSampler())
.Build())
{
Activity parent = null;
if (createParentActivity)
{
parent = new Activity("parent")
.SetIdFormat(ActivityIdFormat.W3C)
.Start();
parent.TraceStateString = "k1=v1,k2=v2";
parent.ActivityTraceFlags = ActivityTraceFlags.Recorded;
parentContext = parent.Context;
}
var request = new HttpRequestMessage
{
RequestUri = new Uri(this.url),
Method = new HttpMethod("GET"),
};
using var c = new HttpClient();
await c.SendAsync(request);
parent?.Stop();
}
if (!sample)
{
Assert.Empty(exportedItems);
}
else
{
Assert.Single(exportedItems);
}
// Make sure custom propagator was called.
Assert.True(contextFromPropagator != default);
if (sample)
{
Assert.Equal(contextFromPropagator, exportedItems[0].Context);
}
Sdk.SetDefaultTextMapPropagator(previousDefaultTextMapPropagator);
}
public void Dispose()
{
this.serverLifeTime?.Dispose();