New design for TelemetryHttpModule using ActivitySource + OpenTelemetry.API #2249
Conversation
| /// <summary> | ||
| /// <see cref="Activity.OperationName"/> for OpenTelemetry.Instrumentation.AspNet created <see cref="Activity"/> objects. | ||
| /// </summary> | ||
| public const string AspNetActivityName = "Microsoft.AspNet.HttpReqIn"; |
There was a problem hiding this comment.
Do we really need this to be a public const?
Public consts are actually "linked into" the target assembly, so if we change the value and do a binplace (rather than recompile), there will be a mismatch. The normal practice I've seen is that we only put something that are truly constants - for example PI = 3.1415926.
There was a problem hiding this comment.
They don't need to be public. The reason I did that was so our instrumentation can use them and we don't need a magic string. We really only need the ActivitySource name (not AspNetActivityName). What was interesting is that the public api analyzer picked them up. So I guess that is its way of saying: if we make them public they are part of the public api and we should never change them. Really changing them would be breaking, so I kind of like them being part of the contract 🤷
| /// extract <see cref="PropagationContext"/> from incoming requests. | ||
| /// </summary> | ||
| [EditorBrowsable(EditorBrowsableState.Never)] | ||
| public TraceContextPropagator TextMapPropagator |
There was a problem hiding this comment.
Do we need this? We can simply use Propagators.DefaultTextMapPropagator ?
There was a problem hiding this comment.
I think we do because Propagators.DefaultTextMapPropagator without an SDK will no-op. In order to make this work as a standalone, I had to give it a default. Went with W3C because that is (more or less) what Microsoft.AspNet.TelemetryCorrelation does today. The existing one also looks for Request-Id & Correlation-Context so the new one isn't 100% compatible. I could add a custom default one to bridge the gap.
What I'm planning to do in our AspNet instrumentation is set this to Propagators.DefaultTextMapPropagator so whatever the SDK is using the module will respect.
| @@ -100,7 +138,7 @@ private void Application_BeginRequest(object sender, EventArgs e) | |||
| { | |||
| var context = ((HttpApplication)sender).Context; | |||
| AspNetTelemetryEventSource.Log.TraceCallback("Application_BeginRequest"); | |||
| ActivityHelper.CreateRootActivity(context, this.ParseHeaders); | |||
| ActivityHelper.StartAspNetActivity(this.TextMapPropagator, context, this.OnRequestStartedCallback); | |||
There was a problem hiding this comment.
not a blocking comment, just sharing some thoughts -- this somewhat makes this a "tracing" only module, as one cannot write a MetricInstrumentation (without tracing).
The CallBacks could be made independent of activity, so a metric-only user can leverage them and build metrics out without creating activity.
There was a problem hiding this comment.
Good call. I could drop the Activity param from the callbacks and we could get it via Activity.Current in AspNet instrumentation. What I was thinking wrt to metrics is we would add a Meter in here eventually and then some instruments which we would also listen to from our instrumentation. The goal being to have TelemetryHttpModule emitting traces and metrics for anyone that wants to listen. What do you think about that?
/cc @alanwest
CodeBlanch
merged commit dfc8e6d
into
open-telemetry:aspnet-telemetrycorrelation-otelintegration
CodeBlanch
deleted the
aspnet-telemetrycorrelation-otelintegration-part1
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]>
[Note: The destination for this PR is a working branch I created so I could do this in multiple smaller[ish] PRs instead of one giant code bomb.]
Changes
TelemetryHttpModulenow usesActivitySource.TraceContextPropagator TextMapPropagatorproperty onTelemetryHttpModulethat our SDK (or users) can plug into to change the propagation style.TelemetryHttpModulethat our AspNet instrumentation (or users) can plug into to do enrichment and tagging.Notes
The reason we made OpenTelemetry.Instrumentation.AspNet.TelemetryHttpModule its own assembly was so existing users of Microsoft.AspNet.TelemetryCorrelation could migrate without necessarily needing all of OpenTelemetry. Here I have added a dependency on OpenTelemetry.Api so did we break that? I'm going to say: no. You don't need the SDK to use the TelemetryHttpModule. You can still use it as a standalone and it will default to
TraceContextPropagatorand work largely as it did before. All the dependency does is let us call into the existing code for propagation (textmap & baggage), we're not using the TracerProvider or anything like that.