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

Allow access to the operation id #1338

Closed
GeeWee opened this issue Dec 4, 2019 · 8 comments
Closed

Allow access to the operation id #1338

GeeWee opened this issue Dec 4, 2019 · 8 comments
Labels

Comments

@GeeWee
Copy link

GeeWee commented Dec 4, 2019

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
Copy link
Contributor

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
Copy link
Member

lmolkova commented Dec 4, 2019

@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
Copy link
Author

GeeWee commented Dec 5, 2019

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
Copy link
Member

lmolkova commented Dec 5, 2019

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
Copy link
Author

GeeWee commented Dec 6, 2019

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
Copy link
Member

lmolkova commented Dec 6, 2019

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
Copy link
Author

GeeWee commented Dec 17, 2019

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
Copy link

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.

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

No branches or pull requests

3 participants