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

Added logic to run sampler a second time after http instrumentation libraries #107

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

AsakerMohd
Copy link
Collaborator

@AsakerMohd AsakerMohd commented Sep 18, 2024

Issue #, if available:

Description of changes:
This new function runs the sampler a second time after the needed attributes (such as UrlPath and HttpTarget) are finally available from the http instrumentation libraries. The sampler hooked into the Opentelemetry SDK runs right before any activity is started so for the purposes of our X-Ray sampler, that isn't going to work and breaks the X-Ray functionality. Running it a second time after http instrumentation libraries are run allows us to retain the sampler functionality.

Below is the stack trace showing the sampler running before the instrumentation libraries are started.

   at OpenTelemetry.Sampler.AWS.AWSXRayRemoteSampler.ShouldSample(SamplingParameters& samplingParameters) in /local/home/heayh/pulse-dotnet/opentelemetry-dotnet-contrib/src/OpenTelemetry.Sampler.AWS/AWSXRayRemoteSampler.cs:line 86
   at AWS.Distro.OpenTelemetry.AutoInstrumentation.AlwaysRecordSampler.ShouldSample(SamplingParameters& samplingParameters) in /local/home/heayh/pulse-dotnet/aws-otel-dotnet-instrumentation+sampler/src/AWS.Distro.OpenTelemetry.AutoInstrumentation/AlwaysRecordSampler.cs:line 70
   at OpenTelemetry.Trace.TracerProviderSdk.RunGetRequestedDataOtherSampler(Activity activity)
   at OpenTelemetry.Trace.TracerProviderSdk.<>c__DisplayClass12_1.<.ctor>b__9(Activity activity)
   at System.Diagnostics.ActivitySource.<>c.<NotifyActivityStart>b__24_0(ActivityListener listener, Object obj)
   at System.Diagnostics.SynchronizedList`1.EnumWithAction(Action`2 action, Object arg)
   at System.Diagnostics.ActivitySource.NotifyActivityStart(Activity activity)
   at System.Diagnostics.Activity.Start()

Below is also a comment from AspNetCore instrumentation library mentioning that the required attributes are only available after sampling decision is made:
https://github.com/open-telemetry/opentelemetry-dotnet-contrib/blob/main/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs#L91-L99

Finally, I opened this discussion and in there is a slack thread going through a similar issue:
open-telemetry/opentelemetry-dotnet#5839

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@AsakerMohd AsakerMohd requested a review from a team as a code owner September 18, 2024 20:01
@birojnayak
Copy link
Collaborator

This would have performance impact and when onboarding customer for the first time, this might not be a good experience.. I would stick with HttpContextAccessor for .NET Core and use upstream for ASPNET. Let's follow-up with OTEL maintainers to get http/method into upstream.

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

Successfully merging this pull request may close these issues.

2 participants