New design for TelemetryHttpModule using ActivitySource + OpenTelemetry.API part 5 #2261
Conversation
| @@ -177,32 +164,33 @@ public void Dispose() | |||
| { | |||
| options.Enrich = GetEnrichmentAction(default); | |||
| } | |||
|
|
|||
| options.RecordException = recordException; | |||
| }) | |||
| .AddProcessor(activityProcessor.Object).Build()) | |||
There was a problem hiding this comment.
Is Mock easier/better than InMemoryExporter ?
There was a problem hiding this comment.
“It shines, somewhere, you do not see it” -Kung Fu 🦗 (there's no grasshopper emoji, which is upsetting)
I'm not a fan of using mocking anywhere, TBH. Prefer to always make an API that is testable. My guess would be this test was written before InMemoryExporter existed? In any case, I cleaned it up a bit.
| private static readonly Func<HttpRequest, string, IEnumerable<string>> HttpRequestHeaderValuesGetter = (request, name) => request.Headers.GetValues(name); | ||
| private static readonly ActivitySource AspNetSource = new ActivitySource( | ||
| TelemetryHttpModule.AspNetSourceName, | ||
| typeof(ActivityHelper).Assembly.GetName().Version.ToString()); |
There was a problem hiding this comment.
Probably not directly related to this PR - is there a chance that this line could throw exception?
There was a problem hiding this comment.
Anything is possible 😄 I'm personally not concerned about it. Because of the typeof(ActivityHelper).Assembly. We are loading the assembly of the code that is running. If that assembly is unavailable for some reason, the code wouldn't be running in the first place! We would have blown up with a load exception of some kind long before this executes. But if you want me to add some error handling, happy to do it. Probably some other spots in the repo doing the same so maybe a follow-up PR?
There was a problem hiding this comment.
No need to do it in this PR.
I asked this mainly because I vaguely remember seeing exception thrown from "Assembly.GetName" in production many years ago, and I cannot remember the details. Trying to see if anyone from the community has experience dealing with such corner case.
A quick Google search "Assembly.GetName() throw exception" gives some clue that SecurityException might happen. Given here what we essentially do is Assembly.GetExecutingAssembly().GetName(), I guess we won't hit the problem like you (@CodeBlanch) mentioned.
CodeBlanch
merged commit 2752246
into
open-telemetry:aspnet-telemetrycorrelation-otelintegration
CodeBlanch
deleted the
aspnet-telemetrycorrelation-otelintegration-part5
branch
…e + OpenTelemetry.API (#2270) * New design for TelemetryHttpModule using ActivitySource + OpenTelemetry.API. (#2249) * New design for TelemetryHttpModule using ActivitySource + OpenTelemetry.API part 2 (#2254) * Use a single context.Items key for state management to make things more efficient. * Added a comment for clarity. * Code review. * New design for TelemetryHttpModule using ActivitySource + OpenTelemetry.API part 3 (#2256) * Update ASP.NET instrumentation to use the new TelemetryHttpModule. * Fixed TelemetryHttpModule not starting its Activity objects. Added an example of request suppression. * Tweaks an logging improvements. * Sealed AspNetInstrumentationEventSource. * Code review. * New design for TelemetryHttpModule using ActivitySource + OpenTelemetry.API part 4 (#2258) * Fixed up TelemetryHttpModule unit tests. * Added tests for the new HasStarted helper and added checks for StartedButNotSampledObj when not sampled. * Code review. * New design for TelemetryHttpModule using ActivitySource + OpenTelemetry.API part 5 (#2261) * Updated ASP.NET instrumentation tests for new TelemetryHttpModule. * Added a test for the new RecordException option. * Code review. * New design for TelemetryHttpModule using ActivitySource + OpenTelemetry.API part 6 (#2264) * CHANGELOG & README updates. * Apply suggestions from code review Co-authored-by: Reiley Yang <[email protected]> Co-authored-by: Reiley Yang <[email protected]> * Lint + sanity checks. * Lint attempt 2. * Restored CHANGELOG changes lost in merge. Co-authored-by: Reiley Yang <[email protected]>
Relates to #2249
Changes