microsoft / ApplicationInsights-dotnet

ApplicationInsights-dotnet
MIT License
565 stars 287 forks source link

Allow access to the operation id #1338

Closed GeeWee closed 3 years ago

GeeWee commented 4 years ago

I'm doing some Serilog integration currently in ASP.NET Core 3, where it would be really nice to be able to access the current operation_id and operation_ParentId, to ship that off along with the traces sent to Application Insights. Particularly in a situation where we use telemetryClient.StartOperation<RequestTelemetry>()

An earlier issue , suggested System.Diagnostics.Activity.Current?.RootId - but that's not accurate anymore. Looking through the codebase it seems like the operation_Id is equivalent to Activity.TraceId and the operation_ParentId is constructed via $"|{activity.TraceId}.{activity.Parent.SpanId}." inside W3CUtilities.FormatTelemetryId

Is this: a) Correct? b) Guaranteed to be stable, if I end up creating this logic into my logging integration c) Or it is perhaps possible to expose the functionality for getting these ID's, so we don't have to go through all of these hoops

cijothomas commented 4 years ago

OperationId in application insights will be activity.traceid. ParentId is $"|{activity.TraceId}.{activity.Parent.SpanId}" to maintain backward compatibility with old SDKs. I believe parentid will be replaced with just activity.parent.spanid in the future, when request.id,dependency.id will be made activity.current.spanid instead of the current "|traceid.spanid"

If you use the SDK with default settings, then all traces (including ilogger ones) should get automatically tagged with correct parentid. Is this not what you see already? Perhaps you are hitting this issue (https://github.com/microsoft/ApplicationInsights-dotnet-logging/issues/305) ?

@lmolkova is the expert here to help more.

lmolkova commented 4 years ago

@GeeWee you should always make sure you configure OperationCorrelationTelemetryInititalizer and never stamp operation_Id, parentId or id on your own.

OperationCorrelationTelemetryInitializer will make sure telemtery is initialized properly.

GeeWee commented 4 years ago

That makes sense. The reason I'm asking about this, is because the Serilog AI Sink doesn't seem to properly instrument the log calls with operation_id or operation_parentid, and I was hoping I could simply monkey-patch it in with a custom logging enricher.

However, what you're saying is that there's potentially something more fundamental wrong in the sink, that should be fixed instead of monkeypatched?

lmolkova commented 4 years ago

Looking into the sink, it seems it uses TelemetryConfiguration.Active which always has OperationCorrelationTelemetryInitializer. However, I might have missed something, or maybe Serilog sink overrides ids somehow. Can you post here example of ids you see with serilog?

GeeWee commented 4 years ago

Yes, let's try to explore the sink issue. Perhaps this is more of an issue over there - if we manage to resolve this, I'll make a PR over there.

While the documentation says to use Telemetry.Active, there's also a different constructor that people have been playing around with in #121. We're not sure TelemetryConfiguration.Active is safe to use, seeing as the configuration made in ASP.Net Core's Configure might not be applied.

Digging a little deeper in the different constructors, I find I've been using the following one in the sink

// Inject IOptionsMonitor<TelemetryConfiguration> inside Startup.Configure
            var loggerConfiguration = LoggerConfiguration
                // other stuff
                .WriteTo.ApplicationInsights(telemetryConfiguration, TelemetryConverter.Traces);

This leads to the following code. var client = new TelemetryClient(telemetryConfiguration) Which is then used internally in the sink. Is this an inaccurate way of constructing the TelemetryClient?

As alternatives, I've just now tried the sink constructor that uses the ASP.Net Core TelemetryClient and uses that, but also the one that uses TelemetryConfiguration.Active - both send what I assume are the correct operational_id's, though I haven't double-checked. When using the constructor above, no operation_ids of any kind are sent.

lmolkova commented 4 years ago

We're not sure TelemetryConfiguration.Active is safe to use, seeing as the configuration made in ASP.Net Core's Configure might not be applied.

Do not use Active if you can avoid it. AI SDK attempts to initialize it in the same way as one in DI, but any customization may lead to discrepancies. With logger adapters it's not always possible NOT to use it though.

var client = new TelemetryClient(telemetryConfiguration)

This is the proper way to initialize TelemteryClient. What I'm mostly interested in is where telemetryConfiguration instance came from and how it was initialized. My guess is that that configuration does not have OperationCorrelationTelemetryInitializer - you could always check the list of initializers on the configuration. Do you own this instance or it's created by the sink? If you own it - create it with TelemteryConfiguration.CreateDefault() rather than parameterless constructor. If you don't - this is worth fixing in the sink. As a workaround you could always add initializer to any instance of TelemteryConfiguration.

As alternatives, I've just now tried the sink constructor that uses the ASP.Net Core TelemetryClient and uses that, but also the one that uses TelemetryConfiguration.Active - both send what I assume are the correct operational_id's, though I haven't double-checked.

This is also a reasonable workaround.

Let me know if it anwsers your question. Thanks!

GeeWee commented 4 years ago

Alright, so I think I've tried to dig up the problem.

The instance owns the sink, but there's a mismatch in the TelemetryInitializers between the ASP.Net core injected TelemetryClient and the TelemetryConfiguration.

If In Configure I inject both TelemetryClient and IOptionsMonitor<TelemetryConfiguration>, I see the following TelemetryInitializers on the the TelemetryConfiguration

[0] = {Microsoft.ApplicationInsights.AspNetCore.TelemetryInitializers.ClientIpHeaderTelemetryInitializer}
[1] = {Microsoft.ApplicationInsights.AspNetCore.TelemetryInitializers.OperationNameTelemetryInitializer}
[2] = {Microsoft.ApplicationInsights.AspNetCore.TelemetryInitializers.SyntheticTelemetryInitializer}
[3] = {Microsoft.ApplicationInsights.AspNetCore.TelemetryInitializers.WebSessionTelemetryInitializer}
[4] = {Microsoft.ApplicationInsights.AspNetCore.TelemetryInitializers.WebUserTelemetryInitializer}
[5] = {Microsoft.ApplicationInsights.AspNetCore.TelemetryInitializers.AspNetCoreEnvironmentTelemetryInitializer}
[6] = {Microsoft.ApplicationInsights.AspNetCore.TelemetryInitializers.DomainNameRoleInstanceTelemetryInitializer}
[7] = {Microsoft.ApplicationInsights.WindowsServer.AzureWebAppRoleEnvironmentTelemetryInitializer}
[8] = {Microsoft.ApplicationInsights.DependencyCollector.HttpDependenciesParsingTelemetryInitializer}
[9] = {Microsoft.ApplicationInsights.AspNetCore.TelemetryInitializers.ComponentVersionTelemetryInitializer}

And the following on the TelemetryClient

[0] = {Microsoft.ApplicationInsights.Extensibility.OperationCorrelationTelemetryInitializer}
[1] = {Microsoft.ApplicationInsights.AspNetCore.TelemetryInitializers.ClientIpHeaderTelemetryInitializer}
[2] = {Microsoft.ApplicationInsights.AspNetCore.TelemetryInitializers.OperationNameTelemetryInitializer}
[3] = {Microsoft.ApplicationInsights.AspNetCore.TelemetryInitializers.SyntheticTelemetryInitializer}
[4] = {Microsoft.ApplicationInsights.AspNetCore.TelemetryInitializers.WebSessionTelemetryInitializer}
[5] = {Microsoft.ApplicationInsights.AspNetCore.TelemetryInitializers.WebUserTelemetryInitializer}
[6] = {Microsoft.ApplicationInsights.AspNetCore.TelemetryInitializers.AspNetCoreEnvironmentTelemetryInitializer}
[7] = {Microsoft.ApplicationInsights.AspNetCore.TelemetryInitializers.DomainNameRoleInstanceTelemetryInitializer}
[8] = {Microsoft.ApplicationInsights.WindowsServer.AzureWebAppRoleEnvironmentTelemetryInitializer}
[9] = {Microsoft.ApplicationInsights.DependencyCollector.HttpDependenciesParsingTelemetryInitializer}
[10] = {Microsoft.ApplicationInsights.AspNetCore.TelemetryInitializers.ComponentVersionTelemetryInitializer}

So the Configuration is missing the OperationCorrelationTelemetryInitializer, that is attached to the client.

github-actions[bot] commented 3 years ago

This issue is stale because it has been open 300 days with no activity. Remove stale label or comment or this will be closed in 7 days.