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

Make BASE SDK Use W3C Correlation by default. #1193

Merged
merged 21 commits into from
Aug 21, 2019
Merged

Conversation

cijothomas
Copy link
Contributor

@cijothomas cijothomas commented Aug 16, 2019

Application Insights used Request-ID based (aka Hierarchical) model to propagate correlation headers between services. i.e it parsed the header 'Request-Id' for incoming correlation ids, and populated the same header for propagating correlations ids to outbound services.

https://github.com/dotnet/corefx/blob/master/src/System.Diagnostics.DiagnosticSource/src/HttpCorrelationProtocol.md

While application insights SDKs also contained the capability to use the new W3C Standard for Distributed Tracing (https://www.w3.org/TR/trace-context/), but this was not enabled by default. Also the implementation had to work around the fact the the .NET correlation primitive Activity did not natively support W3C based correlation. (it only supported Hierrachial model) So SDK used Activity.Tags for storing W3C concepts like TraceID, Spand ID, etc which was inefficient.

Starting with System.Diagnostics.DiagnosticSource package 4.6.0, which this SDK is taking dependency on, Activity now natively support W3C Tracing Distributed Tracing. Activity now has methods like TraceID (https://www.w3.org/TR/trace-context/#trace-id), SpanID(https://www.w3.org/TR/trace-context/#parent-id) which make it fully W3C Tracing aware.

Though Activity added native support for W3C based correlation, the default format is still Hierarchical.

This PR introduces the following changes:

  1. Introduce a new public field on TelemetryConfiguration named EnableW3CCorrelation which will be set to true by default. True on this field sets the default ID Format used on System.Diagnostics.Activity to be W3C.
    Users can set the above to be false, to make SDK fall-back to the old way of correlation using Hierarchical ids.
    If any issues are faced with new format, use this to fall-back to original behavior

  2. OperationCorrelationTelemetryInitializer, which does the in-proc correlation of telemetry is now made W3C-Tracing aware. (Described more at the end of this)

  3. Few classes and methods like W3CCorrelationTelemetryInitializer, W3CActivityExtensions, which were used previously to support W3C tracing are no longer needed and are marked obsolete. Some of them methods are not relevant and are made no-ops.

OperationCorrelationTelemetryInitializer
This class was responsible for correlating telemetry items to parent telemetry item (either Request Telemetry or Dependency Telemetry). This is done by setting their operation.id and operation.parentid from Activity.Current. More specifically, operation.id was set to current Activity's RootID, following (https://github.com/dotnet/corefx/blob/master/src/System.Diagnostics.DiagnosticSource/src/HttpCorrelationProtocol.md), and operation.ParentId was set to current Activity's ID.

With the new changes in this PR, the default behavior is to set IDs in the W3C Tracing format. Specifically, this means operation.ID will be set to current Activity's TraceID, and operation.ParentID will be set to {!TraceID.SpanId.} from current Activity. This format for parent id is done to ensure backward compatibility with older SDKs excepting ids in Hierarchical format.

TelemetryClient extension methods StartOperation
These methods provided a way for users to track a custom operation telemetry (as opposed to auto collected Request/Dependency). This will now generate IDs in the new W3C compliant format.
If you used to provide your own operationid for the StartOperation() overload which accepts operationid, it will be used only if they are compatible with W3C Ids. (16 byte hex encoded string) Otherwisde a new compatible ID is generated, and the provided id will be stored in custom property "ai_legacyRootId". ApplicationInsights UI (app map/e2e transaction views) are aware of this custom field, and hence there should be no degradation in the final experience.

Read more about W3C Standard For Distributed Tracing here - https://www.w3.org/TR/trace-context/

There will be related changes in other components of application insights SDK - in the Request Collection Module, and in DependencyCollection Modules.

Additional notes:
All changes are backward compatible - components instrumented with this new SDK will continue to work with components instrumented with older version of the SDK.

@cijothomas cijothomas marked this pull request as ready for review August 19, 2019 23:39
{
// There is no need of checking if TelemetryContext.ID is W3C Compatible. It is always set to
// W3C compatible id. Even user supplied non-compatible ID is ignored.
operationActivity.SetParentId(string.Join("-", W3CConstants.DefaultVersion, telemetryContext.Id, W3CConstants.InvalidSpanID, W3CConstants.DefaultTraceFlag));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to make it cleaner you can do this:
SetParentId(ActivityTraceId.CreateFromString(telemetryContext.Id.AsSpan()), default, ActivityTraceFlags.None);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I opted for this because I cannot find a way to create a Default SpanID. as SpanId.CreateFromString will throw for all 0s. An alternate is to do new Activity().SpanId, which gives a SpanId with all zeroes, and cache this. Will discuss in person about this.

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

Successfully merging this pull request may close these issues.

3 participants