[Do not Merge] Testing net7.0 HttpClient with ActivitySourceListener #3016
Conversation
…ub.com/vishweshbankwar/opentelemetry-dotnet into vibankwa/try-httpclient-activitysource
Codecov Report
@@ Coverage Diff @@
## main #3016 +/- ##
==========================================
+ Coverage 85.39% 85.42% +0.03%
==========================================
Files 270 270
Lines 9561 9581 +20
==========================================
+ Hits 8165 8185 +20
Misses 1396 1396
|
|
This PR was marked stale due to lack of activity and will be closed in 7 days. Commenting or Pushing will instruct the bot to automatically remove the label. This bot runs once per day. |
github-actions
bot
added
the
Stale
|
Closed as inactive. Feel free to reopen if this PR is still being worked on. |
vishweshbankwar
changed the title
src/OpenTelemetry.Instrumentation.Http/Implementation/HttpHandlerDiagnosticListener.cs
Outdated
Show resolved
Hide resolved
| @@ -25,13 +25,37 @@ internal class HttpClientInstrumentation : IDisposable | |||
| { | |||
| private readonly DiagnosticSourceSubscriber diagnosticSourceSubscriber; | |||
|
|
|||
| private readonly Func<string, object, object, bool> isEnabled = (activityName, obj1, obj2) => | |||
There was a problem hiding this comment.
nit:
private readonly Func<string, object, object, bool> isEnabled = (activityName, obj1, obj2) => activityName == "System.Net.Http.HttpRequestOut";
| internal static readonly Version Version = AssemblyName.Version; | ||
| internal static readonly ActivitySource ActivitySource = new(ActivitySourceName, Version.ToString()); | ||
| internal static readonly bool IsNet7OrGreater = typeof(HttpClient).Assembly.GetName().Version.Major >= 7; |
There was a problem hiding this comment.
nit: IsHttpClientVersion7OrGreater?
| internal static readonly Version Version = AssemblyName.Version; | ||
| internal static readonly ActivitySource ActivitySource = new(ActivitySourceName, Version.ToString()); | ||
| internal static readonly bool IsNet7OrGreater = typeof(HttpClient).Assembly.GetName().Version.Major >= 7; |
There was a problem hiding this comment.
This check looks ok to me. I double checked the supported versions of http client, and this check is good enough. The other idea can be checking Environment.Version to know if running on .NET 7.0 or later as http client 7.0 will be used only on .NET 7.0. But I don't prefer this option as in the future http client can decide to run down-level versions including netfx.
One other idea can be, enable DiagnosticSource listener till you get the first http client activity and then check the Activity.Source as @noahfalk suggested. Then if the Source is not null, turn off DiagnosticSource listener on http client and enable ActivitySource for it. Maybe this will immune you against versioning changes/risks.
There was a problem hiding this comment.
I think the check as-is in this PR is a good tradeoff between simplicity and reliability. My suggestion to use the Activity.Source property was assuming we were running logic in a callback that had the Activity passed in and that no caching would be necessary. Trying to do that check in this case feels unnecessarily complex.
There was a problem hiding this comment.
Thanks @tarekgh @noahfalk - The second option could introduce an issue - Thinking about the edge case - If I have sampler always returning ActivitySamplingResult.None. In this case HttpClient will fall back to check if there is a diagnostic listener present, if there is one it will create activity using new Activity(). For this case, we will never get an activity with source set.
I am checking source property as well here as there could be another diagnosticsource listener present (e.g. another instrumentation library) which could ultimately cause activity to be created.
Please correct me if I am wrong.
There was a problem hiding this comment.
I think this PR is doing the right things : )
|
Closed as inactive. Feel free to reopen if this PR is still being worked on. |
Fixes #3018