Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.

[release/3.1] DiagnosticSourceEventSource fixes for distributed tracing #42104

Merged
merged 2 commits into from Oct 30, 2019

Conversation

noahfalk
Copy link
Member

@noahfalk noahfalk commented Oct 25, 2019

Ported change: #41943
Approved API Proposal: NA - no new APIs

Description

We are trying to aid Azure scenarios to light up distributed tracing in a painless way. This set of changes provides additional functionality out-of-the box for any customer running .Net Core. There are three elements within the fix:

  1. There is no existing mechanism for DiagnosticSourceEventSource to fetch the
    current activity object. By convention it is not passed as an event argument,
    but rather stored in the async-local property Activity.Current. This was fixed
    by adding a well-known "*Activity" property that returns the result of
    Activity.Current regardless what object it is applied to.

  2. DiagnosticSourceEventSource fails to evaluate properties on value types, such
    as DateTime.Ticks. Calling MethodInfo.CreateDelegate needs to use a different
    signature for ref and value type properties and previously the code always used
    the ref-style signature. Fixed by adding ValueTypedFetchProperty that does the
    proper CreateDelegate and delegate invocation for structs.

  3. There is no mechanism for DiagnosticSourceEventSource to enumerate the tags
    on an activity. This change adds the *Enumerate well-known property which will
    iterate any IEnumerable`1, invoke ToString() on each element, then join it as a
    string.

Customer impact

Currently the standard solution for distributed tracing requires the developer to pre-req a NuGet package for ApplicationInsights (or other telemetry provider). Many customers don't do this, or include versions that are overly old. For the developer tasked with diagnosing a production issue later, solutions that require an application to be redployed or dependencies changed is high friction, if not impossible. The solution here allows a telemetry agent deployed automatically as part of an Azure PaaS solution to connect to the .Net Core app and extract the relevant distributed tracing telemetry with no effort from the app developer. This in turn makes it much more likely that the telemetry data is available when production issues need to be diagnosed.

Regression

This change will not cause a regression.

Risk

Small to Medium:
There is a bit of code churn introduced by the changes with a proportional opportunity for bugs to lurk. A few factors mitigate this:
a) I've added a test that hits all the new cases and debugged through it
b) DiagnosticSourceEventSource has only a niche set of tooling that consumes it
c) The two new feature options need the telemetry tool to opt-in before they are engaged. If it turned out they were broken tools could leave them off and be no worse than now.

Testing
Added unit test which covers the new code. Not included here I also have an E2E scenario prototype which I confirmed worked

1) There is no existing mechanism for DiagnosticSourceEventSource to fetch the
current activity object. By convention it is not passed as an event argument,
but rather stored in the async-local property Activity.Current. This was fixed
by adding a well-known "*Activity" property that returns the result of
Activity.Current regardless what object it is applied to.

2) DiagnosticSourceEventSource fails to evaluate properties on value types, such
as DateTime.Ticks. Calling MethodInfo.CreateDelegate needs to use a different
signature for ref and value type properties and previously the code always used
the ref-style signature. Fixed by adding ValueTypedFetchProperty that does the
proper CreateDelegate and delegate invocation for structs.

3) There is no mechanism for DiagnosticSourceEventSource to enumerate the tags
on an activity. This change adds the *Enumerate well-known property which will
iterate any IEnumerable`1, invoke ToString() on each element, then join it as a
string.
@stephentoub stephentoub changed the title DiagnosticSourceEventSource fixes for distributed tracing [release/3.1] DiagnosticSourceEventSource fixes for distributed tracing Oct 25, 2019
@stephentoub stephentoub added the Servicing-consider Issue for next servicing release review label Oct 25, 2019
@danmoseley danmoseley added this to the 3.1 milestone Oct 25, 2019
@danmoseley danmoseley added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Oct 30, 2019
@danmoseley
Copy link
Member

Please get a review before merging 😃

@noahfalk
Copy link
Member Author

@stephentoub - you reviewed the original change so hopefully this is pretty straightforward : )

Copy link
Member

@danmoseley danmoseley left a comment

Choose a reason for hiding this comment

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

appears to be a faithful port of https://github.com/dotnet/corefx/pull/41943/files

@danmoseley danmoseley merged commit 0f5d481 into dotnet:release/3.1 Oct 30, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
4 participants