Support W3C distributed tracing standard by lmolkova · Pull Request #945 · microsoft/ApplicationInsights-dotnet-server

lmolkova · 2018-06-30T00:15:01Z

Implements w3c distributed tracing standard.

This change enables opt-in W3C support via settings.
To enable it, add W3COperationCorrelaitonTelemetryInitilizer and set RequestTrackingTelemetryModule.EnableW3CHeadersExtraction and DependencyTrackingTelemetryModule.EnableW3CHeadersInjection to true.

This change ensures W3C ids are set on the telemetry (operationId, parentId and Id). 'legacy' ids are set in custom dimensions whenever they are different from the W3C ones.

This change requires UI queries change to support querying for custom dimensions for the cases when operation id or parent id do not match on the legacy-W3C boundary.

Out of date please ignore
This is an experimental feature that could be turned on by setting APPLICATIONINSIGHTS_ENABLE_W3C_TRACING environment variable to true.

This is a temporary solution for non-production scenarios to validate W3C tracing standard.
It will be done properly within .NET SDK and Asp.NET frameworks.

For now, ApplicationInsights with enabled W3C support is not backward-compatible: i.e. correlation with other services instrumented with ApplicaitonInsights (but without the W3C) does NOT work.

lmolkova · 2018-07-11T01:51:18Z

@SergeyKanzhelev ping

SergeyKanzhelev

LGTM for experimental support

SergeyKanzhelev · 2018-07-11T16:16:43Z

Src/Common/W3C/W3CActivityExtensions.cs

+    /// <summary>
+    /// Extends Activity to support W3C distributed tracing standard.
+    /// </summary>
+    [Obsolete("Not ready for public consumption.")]


will this Obsolete attribute apply to extension methods inside? I just don't know how it works for extensions...

SergeyKanzhelev · 2018-07-11T16:17:05Z

Src/Common/W3C/W3CActivityExtensions.cs

+    /// Extends Activity to support W3C distributed tracing standard.
+    /// </summary>
+    [Obsolete("Not ready for public consumption.")]
+    [EditorBrowsable(EditorBrowsableState.Never)]


Same question here - should it be applied to individual methods?

SergeyKanzhelev · 2018-07-11T16:18:55Z

Src/Common/W3C/W3CActivityExtensions.cs

+
+        private static string GenerateSpanId()
+        {
+            // inefficient


don't tell =)

SergeyKanzhelev · 2018-07-11T16:20:20Z

Src/Common/W3C/W3CConstants.cs

+        /// <summary>
+        /// Default sampled flag value.
+        /// </summary>
+        internal const string DefaultSampled = "01";


I'm not sure about this one... We discussed 0 to be "decide for yourself" so maybe 0 is a better default.

lmolkova · 2018-08-07T17:02:10Z

@SergeyKanzhelev @reyang @MS-TimothyMothra this is finally ready for the review. Please have a look

reyang · 2018-08-07T17:08:47Z

Src/Common/InjectionGuardConstants.cs

+        /// <summary>
+        /// Max number of key value pairs in the tracestate header.
+        /// </summary>
+        public const int TraceStateMaxPairs = 128;


128 -> 32

https://github.com/w3c/distributed-tracing/blob/master/trace_context/HTTP_HEADER_FORMAT.md#header-value

Bogdan has changed the max number of key-value-pairs to 32:

list = list-member 0*31( OWS "," OWS list-member ) list-member = key "=" value

reyang · 2018-08-07T17:16:13Z

Src/Common/W3C/W3CActivityExtensions.cs

+        }
+
+        /// <summary>
+        /// Intializes W3C context on the Activity from traceparent header value.


minor: Intializes -> Initializes

reyang · 2018-08-07T17:17:28Z

Src/Common/W3C/W3CActivityExtensions.cs

+            {
+                switch (tag.Key)
+                {
+                    case W3CConstants.TraceIdTag:


minor: sort the case statements either alphabetically or following the physical order of fields in traceparent.

This would make it easier for people to read and maintain the code, similar like how we organize the using namespace statements - not a must have but definitely helpful.

while this makes sense, I do have them ordered based on the importance and think this is an appropriate order.

reyang · 2018-08-07T17:23:43Z

Src/Common/W3C/W3CActivityExtensions.cs

+            if (value != null)
+            {
+                var parts = value.Split('-');
+                if (parts.Length == 4)


Curious, why do we have a check "parts.Length == 4" here, while not checking for the format (silently ignore if length != 4)?

what would be appropriate in this case? If traceparent was invalid, there is not much we could do - new traceid/span id will be generated later.

Keep in mind, this is not a real implementation, just a temporary one, the real one will be in .NET and this implementation will go away. Until that, I prefer to keep things simple and loose and avoid too much validation.

reyang · 2018-08-07T17:24:30Z

Src/Common/W3C/W3CActivityExtensions.cs

+                var parts = value.Split('-');
+                if (parts.Length == 4)
+                {
+                    activity.SetVersion(parts[0]);


minor, sort this alphabetically or physically (according to the order of 'parts')

why is the order important?

reyang · 2018-08-07T17:32:51Z

Src/Common/W3C/W3CConstants.cs

+        /// <summary>
+        /// TraceState tag name.
+        /// </summary>
+        internal const string TraceStateTag = "w3c_traceState";


Suggest using lowercase "tracestate", please refer to the discussion census-instrumentation/opencensus-python#238.
Basically Bogdan and Sergey suggested to treat tracestate as a single word instead of TraceState.

this is internal stuff

c# coding style suggest having const variables in PascalCase and we should follow specific language coding practices.

I suggest that we treat both internal and external names the same way, for consistency.

Yes, we should follow PascalCase, and this is not the point here. The point is that in W3C we treat traceparent and tracestate as a "monolithic word", so the corresponding Pascal case should be Traceparent and Tracestate.

reyang · 2018-08-07T17:36:23Z

Src/Common/WebHeaderCollectionExtensions.cs

-            IEnumerable<string> headerValue = GetHeaderValue(headers, headerName);
-            headers[headerName] = string.Join(", ", HeadersUtilities.UpdateHeaderWithKeyValue(headerValue, keyName, value));
+            IEnumerable<string> headerValue = headers.GetHeaderValue(headerName);
+            headers[headerName] = string.Join(",", HeadersUtilities.UpdateHeaderWithKeyValue(headerValue, keyName, value));


curious, why do we remove the OWS (optional white space) here?

headers are not compressed in HTTP 1.1. and this is not intended to be humar readable anyway.

let me rephrase my question, why do we do it in this pull request? Is this related to the W3C implementation, or it's some optimization we want to make?

we do this in this pull request because it's an obvious, nice and easy thing to do and improves things around tracestate implementation. I agree we may do it in the other PR, but being so subtle. it does not worth the hassle from my point of view

reyang · 2018-08-10T02:22:26Z

Src/Common/W3C/W3COperationCorrelationTelemetryInitializer.cs

+            }
+        }
+    }
+}


minor: putting an empty line before EOF

we have a stylecop that checks such kind of issues and it did not complain. If you think having empty line at the end of file is useful, please modify the rule and apply it to all files :)

…perties

SergeyKanzhelev · 2018-08-13T20:10:29Z

Src/Common/StringUtilities.cs

    /// </summary>
-    public static class StringUtilities
+#if DEPENDENCY_COLLECTOR
+    public


I like hiding this class from public surface. What's the reason to keep it public?

this class is used by AspNetCore SDK and should be visible to it. There are two alternatives:

copy paste (as it is done now and i'm getting read of it by this PR)

move common code to based SDK. I'm not fond of it because most of W3C code will go away from AI SDK eventually and I'd prefer to keep it all in one place. We can move other StringUtilities common methods in different PR

…oft/ApplicationInsights-dotnet-server into lmolkova/W3CSupportOnCore

SergeyKanzhelev · 2018-08-13T21:57:57Z

Src/Common/W3C/W3CActivityExtensions.cs

+
+                    byte.TryParse(parts[3], NumberStyles.HexNumber, CultureInfo.InvariantCulture, out var sampled);
+
+                    if (traceId.Length == 32 && parentSpanId.Length == 16)


we'll need to check for characters to be hex.

the spec now says that app MAY ignore the traceid/spanid if they are not valid. I remember we discussed that implementation will have to validate traceis/spanid and ignore invalid ones. I'll be happy to do this after spec will tell so :)

SergeyKanzhelev · 2018-08-13T21:59:18Z

Src/Common/W3C/W3CConstants.cs

+        /// <summary>
+        /// Name of the field that carry ApplicationInsights application Id in the tracestate header.
+        /// </summary>
+        public const string ApplicationIdTraceStateField = "@msappid";


recommendation is to add @ for the vendor name in case of multi-tenant. For a single tenant - this delimiter is not needed.

Also for azure we might need more than just app-id

ok, so do we need to add @ now? do we know what else we need right now?

SergeyKanzhelev

minor comments

lmolkova requested review from SergeyKanzhelev and TimothyMothra June 30, 2018 00:17

lmolkova self-assigned this Jun 30, 2018

SergeyKanzhelev approved these changes Jul 11, 2018

View reviewed changes

lmolkova force-pushed the lmolkova/W3CSupportOnCore branch from a983ad3 to 590a89a Compare August 2, 2018 01:36

lmolkova changed the title ~~Experimental: Support W3C distributed tracing standard~~ [DO NOT MERGE] Experimental: Support W3C distributed tracing standard Aug 2, 2018

lmolkova force-pushed the lmolkova/W3CSupportOnCore branch from 92a2a1d to 85fd36a Compare August 2, 2018 19:43

lmolkova changed the title ~~[DO NOT MERGE] Experimental: Support W3C distributed tracing standard~~ Experimental: Support W3C distributed tracing standard Aug 7, 2018

lmolkova changed the title ~~Experimental: Support W3C distributed tracing standard~~ Support W3C distributed tracing standard Aug 7, 2018

lmolkova mentioned this pull request Aug 7, 2018

Support W3C incoming headers in opt-in mode microsoft/ApplicationInsights-aspnetcore#735

Merged

reyang reviewed Aug 8, 2018

View reviewed changes

lmolkova mentioned this pull request Aug 10, 2018

Copy parent tags to the new Activity when parent is lost aspnet/Microsoft.AspNet.TelemetryCorrelation#30

Merged

reyang reviewed Aug 10, 2018

View reviewed changes

Liudmila Molkova added 15 commits August 10, 2018 12:08

Support W3C propagation in the process and http injection

ffcff4c

Set context by default

c828db3

fixes

cfdc21f

Enable W3C on desktop: requests and dependencies

bf8a8c4

fix broken tests

070fe7d

helpers and fixes

57ef672

fix no w3c context in parent

10dc70a

changelog and more tests

f330c04

review comments, tracestate proper ordering and tracestate in the pro…

603e925

…perties

0 default sampling

43a344a

correlation backward compatibility via legacy ids in custom dimensions

0cb176a

Enable W3C via setting, not env var

fa60df5

minor fixes

a91c71a

fix stylecop

2ebed64

limit visibility in common items

aec4fc4

Liudmila Molkova added 6 commits August 10, 2018 12:12

public w3c stuff

5c6b998

minor fixes

8d4f703

minor fixes

61aac47

some validation

f627813

more validations and code review

34cdab0

Tracestate and Traceparent

4f229b1

lmolkova force-pushed the lmolkova/W3CSupportOnCore branch from 112de84 to 4f229b1 Compare August 10, 2018 19:26

Liudmila Molkova added 2 commits August 13, 2018 11:10

@ before msappid

6bcad79

Merge branch 'develop' into lmolkova/W3CSupportOnCore

78b4573

SergeyKanzhelev reviewed Aug 13, 2018

View reviewed changes

Liudmila Molkova added 2 commits August 13, 2018 13:16

rename GetTraceState to GetTracestate

08ea8c5

Merge branch 'lmolkova/W3CSupportOnCore' of https://github.com/Micros…

7d67622

…oft/ApplicationInsights-dotnet-server into lmolkova/W3CSupportOnCore

SergeyKanzhelev reviewed Aug 13, 2018

View reviewed changes

SergeyKanzhelev approved these changes Aug 13, 2018

View reviewed changes

Liudmila Molkova added 3 commits August 14, 2018 17:09

traceid and spanid validation, az alias for all azure trace states

5735f66

fix separators

84a561c

Merge branch 'develop' into lmolkova/W3CSupportOnCore

82faad3

lmolkova merged commit 84a30c0 into develop Aug 15, 2018

lmolkova deleted the lmolkova/W3CSupportOnCore branch September 3, 2019 20:45

+                          }
+                      }
+                  }
+              }

                
                    No newline at end of file


		byte.TryParse(parts[3], NumberStyles.HexNumber, CultureInfo.InvariantCulture, out var sampled);

		if (traceId.Length == 32 && parentSpanId.Length == 16)

Conversation

lmolkova commented Jun 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lmolkova commented Jul 11, 2018

Uh oh!

SergeyKanzhelev left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lmolkova commented Aug 7, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SergeyKanzhelev left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

lmolkova commented Jun 30, 2018 •

edited

Loading