-
Notifications
You must be signed in to change notification settings - Fork 67
Support W3C distributed tracing standard #945
Conversation
@SergeyKanzhelev ping |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM for experimental support
/// <summary> | ||
/// Extends Activity to support W3C distributed tracing standard. | ||
/// </summary> | ||
[Obsolete("Not ready for public consumption.")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will this Obsolete attribute apply to extension methods inside? I just don't know how it works for extensions...
/// Extends Activity to support W3C distributed tracing standard. | ||
/// </summary> | ||
[Obsolete("Not ready for public consumption.")] | ||
[EditorBrowsable(EditorBrowsableState.Never)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same question here - should it be applied to individual methods?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
|
||
private static string GenerateSpanId() | ||
{ | ||
// inefficient |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don't tell =)
Src/Common/W3C/W3CConstants.cs
Outdated
/// <summary> | ||
/// Default sampled flag value. | ||
/// </summary> | ||
internal const string DefaultSampled = "01"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure about this one... We discussed 0
to be "decide for yourself" so maybe 0
is a better default.
a983ad3
to
590a89a
Compare
92a2a1d
to
85fd36a
Compare
@SergeyKanzhelev @reyang @MS-TimothyMothra this is finally ready for the review. Please have a look |
/// <summary> | ||
/// Max number of key value pairs in the tracestate header. | ||
/// </summary> | ||
public const int TraceStateMaxPairs = 128; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
128 -> 32
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
} | ||
|
||
/// <summary> | ||
/// Intializes W3C context on the Activity from traceparent header value. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor: Intializes -> Initializes
{ | ||
switch (tag.Key) | ||
{ | ||
case W3CConstants.TraceIdTag: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor: sort the case statements either alphabetically or following the physical order of fields in traceparent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
while this makes sense, I do have them ordered based on the importance and think this is an appropriate order.
if (value != null) | ||
{ | ||
var parts = value.Split('-'); | ||
if (parts.Length == 4) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Curious, why do we have a check "parts.Length == 4" here, while not checking for the format (silently ignore if length != 4)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
var parts = value.Split('-'); | ||
if (parts.Length == 4) | ||
{ | ||
activity.SetVersion(parts[0]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor, sort this alphabetically or physically (according to the order of 'parts')
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is the order important?
Src/Common/W3C/W3CConstants.cs
Outdated
/// <summary> | ||
/// TraceState tag name. | ||
/// </summary> | ||
internal const string TraceStateTag = "w3c_traceState"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- this is internal stuff
- c# coding style suggest having const variables in PascalCase and we should follow specific language coding practices.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- 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.
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)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
curious, why do we remove the OWS (optional white space) here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
headers are not compressed in HTTP 1.1. and this is not intended to be humar readable anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
} | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor: putting an empty line before EOF
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 :)
112de84
to
4f229b1
Compare
@@ -7,7 +7,12 @@ | |||
/// <summary> | |||
/// Generic functions to perform common operations on a string. | |||
/// </summary> | |||
public static class StringUtilities | |||
#if DEPENDENCY_COLLECTOR | |||
public |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like hiding this class from public surface. What's the reason to keep it public?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
|
||
byte.TryParse(parts[3], NumberStyles.HexNumber, CultureInfo.InvariantCulture, out var sampled); | ||
|
||
if (traceId.Length == 32 && parentSpanId.Length == 16) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we'll need to check for characters to be hex
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 :)
Src/Common/W3C/W3CConstants.cs
Outdated
/// <summary> | ||
/// Name of the field that carry ApplicationInsights application Id in the tracestate header. | ||
/// </summary> | ||
public const string ApplicationIdTraceStateField = "@msappid"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
recommendation is to add @
for the vendor name in case of multi-tenant. For a single tenant - this delimiter is not needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also for azure we might need more than just app-id
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, so do we need to add @ now? do we know what else we need right now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor comments
Implements w3c distributed tracing standard.
This change enables opt-in W3C support via settings.
To enable it, add W3COperationCorrelaitonTelemetryInitilizer and set
RequestTrackingTelemetryModule.EnableW3CHeadersExtraction
andDependencyTrackingTelemetryModule.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 totrue
.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.