Skip to content
This repository has been archived by the owner on Jul 5, 2020. It is now read-only.

Leverage W3C implementation from .NET in requests and depedencies collectors #1252

Merged
merged 23 commits into from
Sep 9, 2019

Conversation

lmolkova
Copy link
Member

@lmolkova lmolkova commented Sep 4, 2019

  • Updates DiagnosticSource to the new version (4.6.0-preview)
  • Updates TelemetryCorrelation to 1.0.6
  • Updates base SDK to W3C-enabled
  • Updates tests for W3C (w3c is default now)
  • .NET core 3.0 functional tests are implemented and pass locally, but CI test infra for web is not ready for 3.0, so no auto-test for now
  • A lot of code implementing W3C is removed.
  • Also removed code that dealt with lost activities - with public Activity.Current setter, there is no need anymore

Src/Common/W3C/W3CConstants.cs Show resolved Hide resolved
Src/Web/Web.Shared.Net/Implementation/ActivityHelpers.cs Outdated Show resolved Hide resolved
Src/Web/Web.Shared.Net/Implementation/WebEventSource.cs Outdated Show resolved Hide resolved
break;

case "http":
title = "Made Sync GET HTTP call to bing";
Copy link
Member

Choose a reason for hiding this comment

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

Technically this adds a dependency of out tests reliability on Bing's availability, thus lowering ( :) ) our pass rate in the long run. No op, though, our pass rate is less than Bing's availability :)

Copy link
Member Author

Choose a reason for hiding this comment

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

since this is a copy-paste of other func tests, I'll leave it like that. Bing availability is relatively good compared to our tests stability ;)

Copy link
Contributor

@cijothomas cijothomas left a comment

Choose a reason for hiding this comment

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

review ongoing.

[TestMethod]
public void NetCore30_OnActivityStartInjectsHeaders()
{
var activity = new Activity("System.Net.Http.HttpRequestOut");
Copy link
Contributor

Choose a reason for hiding this comment

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

would be nice to move the name of activity to const.

Copy link
Member Author

Choose a reason for hiding this comment

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

why? this is test

@@ -62,8 +60,14 @@ public class DependencyTrackingTelemetryModule : ITelemetryModule, IDisposable
/// <summary>
/// Gets or sets a value indicating whether to enable W3C distributed tracing headers injection.
/// </summary>
[Obsolete("This field has been deprecated. Please set TelemetryConfiguration.EnableW3CCorrelation.")]
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of TelemetryConfiguration.EnableW3CCorrelation, do
Activity.DefaultIdFormat = ActivityIdFormat.W3C;
Activity.ForceDefaultIdFormat = true;

if (currentActivity.ParentSpanId != default)
{
telemetry.Context.Operation.ParentId = string.Concat('|', traceId, '.',
currentActivity.ParentSpanId.ToHexString(), '.');
Copy link
Contributor

Choose a reason for hiding this comment

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

a private helper method to do ID generation from trace/span ids,

if (!requestHeaders.Contains(RequestResponseHeaders.RequestIdHeader))
{
requestHeaders.Add(RequestResponseHeaders.RequestIdHeader,
string.Concat('|', currentActivity.TraceId.ToHexString(), '.', currentActivity.SpanId.ToHexString(), '.'));
Copy link
Contributor

Choose a reason for hiding this comment

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

helper method to construct requestid

[TestMethod]
public void OnActivityStartInjectsW3COff()
{
this.configuration.EnableW3CCorrelation = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

disable w3c using Activity static fields.

}

/// <summary>
/// Tests that activity without parent id does not get a new W3C compatible root id.
Copy link
Contributor

Choose a reason for hiding this comment

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

with parentid

activity.UpdateTelemetry(telemetry, true);
if (activity.TraceStateString != null && !telemetry.Properties.ContainsKey(W3CConstants.TracestatePropertyKey))
{
telemetry.Properties.Add(W3CConstants.TracestatePropertyKey, activity.TraceStateString);
Copy link
Contributor

Choose a reason for hiding this comment

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

lets do this in base sdk.

Copy link
Member Author

Choose a reason for hiding this comment

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

Src/Common/StringUtilities.cs Show resolved Hide resolved


// TODO move to base SDK?
if (!string.IsNullOrEmpty(currentActivity.TraceStateString) && !telemetry.Properties.ContainsKey(W3CConstants.TracestatePropertyKey))
Copy link
Contributor

Choose a reason for hiding this comment

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

reminder to remove this logic from web as base does this once microsoft/ApplicationInsights-dotnet#1207 merged

@lmolkova lmolkova merged commit 3daab3b into develop Sep 9, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants