Proposed new System.Diagnostics APIs

[tarekgh]

This PR intended to share the proposal of the new APIs we need to add to System.Diagnostics namespace.

The file src/libraries/System.Diagnostics.DiagnosticSource/ref/System.Diagnostics.DiagnosticSourceActivity.cs has the proposed APIs. The rest of the files are just implementation and tests.

[Dotnet-GitSync-Bot]

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

[tarekgh]

@lmolkova @noahfalk @SergeyKanzhelev @pakrym

[tarekgh]

@lmolkova you mentioned we need to have more overloads for this API. could you please list what other parameters should be passed to the overload?

[lmolkova]

this is all possible options (needed/potentially needed for sampling):

https://github.com/open-telemetry/opentelemetry-dotnet/blob/faafd05d3f36f81d4b0f6ea4eaa66e40e3987cc8/src/OpenTelemetry.Api/Trace/Tracer.cs#L70

• name of the Activity - we have it already
• parent context - we have it
• Links - needed for sampling
• Attributes - may be needed for sampling, but looks like it's a future case

This is OpenTelemetry spec on it
https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/api-tracing.md#span-creation

[tarekgh]

Thanks. looks we need overload takes the Links and optional start time.

[tarekgh]

Please look at the commit b380e97 and let me know if what I did is good enough. Thanks.

[lmolkova]

we discussed that for libs (Azure SDK) it's not a problem to create ActivitySource per operation, but it seems it could a problem/typical mistake for app developers.

E.g. imagine in the controller you want to track custom Activities. Developers would have to create static ActivitySource and I guess it will be common forget/not read docs and do this:

using (var s = new ActivitySource("foo"))
using (var a = s.CreateActivity())
{
  // do stuff
}

Can we think about it from app dev convenience?

[tarekgh]

does the concern here ActivitySource is Disposable? if so, we can think in another way to dispose it. but wouldn't anyone writing a code as you showed will not work for them anyway and will it be obvious there is a problem?
One idea we can provide a method like Untrack() (or whatever name we can agree on) and we'll remove the IDisposable interface.

static ActivitySource s = new ActivitySource("foo");
...
using (var a = s.CreateActivity())
{
  // do stuff
}

...
// later if the app want to get rid of it will do 
s.Untrack();

what you think?

[noahfalk]

If an app wrote @lmolkova's example I don't see any reason that it wouldn't function properly? The only downside of that pattern is the app author is paying the cost to create the ActivitySource, publish it, and have telemetry agents decide if they want to subscribe to it on each Activity generated.

Can we think about it from app dev convenience?

If the concern is being able to write simple one liners or not reading the docs then I'd guess the most likely risk is not calling Dispose() on the ActivitySource. For example:

    using(var a = new ActivitySource("foo").CreateActivity())
    {
    }

Looking at it a bit I believe we could eliminate the need for Dispose() by making a global list of weak GCHandles rather than a global list of strong ActivitySource references. We pay one additional pointer of memory per ActivitySource for the GCHandle and need a little more complicated implementation. ActivitySource is already around 100 bytes each (~4 pointers + 3 pointers for string + ~20*2 characters) so another pointer doesn't appear to be a significant relative overhead.

[lmolkova]

The only downside of that pattern is the app author is paying the cost to create the ActivitySource, publish it, and have telemetry agents decide if they want to subscribe to it on each Activity generated.

This is exactly my concern: the easiest and most convenient way to use this API requires 2 allocations and listener to start/stop subscription in e.g. per-request basis (imagine concurrency issues and locks in listener).

Assuming source is less granular that activity, it's possible to have 1) default source and you create one per app or web host 2) source per e.g. controller that you can register as singleton in your container.

With current implementation it's not even possible to register source in DI container (if you want more than one) and users will be forced to have static fields to keep each source.

[tarekgh]

A couple of clarification questions to get the whole picture:

• Does the ask to make it easier to create a source, publish and listen to the custom activities without the need to hold the source in a static field (or store it somewhere)?
• if the answer is yes, does it require at any point later you can retrieve the exactly created source instance to do something else with it? or don't care about the source after establishing the event listeners?

[noahfalk]

Assuming source is less granular that activity, it's possible to have 1) default source and you create one per app or web host 2) source per e.g. controller that you can register as singleton in your container.

I think similar to @tarekgh I am trying to understand your goal : ) Is the goal of the default source to allow for simpler usage like this?

    using(Activity a = Activity.Start("activityName"))
    {
    }

The source per controller I am less sure what the goal is there?

With current implementation it's not even possible to register source in DI container

Is your concern that if we don't then it won't appeal to developers writing ASP.Net apps? Something else?

[SergeyKanzhelev]

with the design with so many allocations - is the assumption that the span name for ASP.NET incoming requests will be a constant value like "HttpIn"? In OpenTelemetry there is a notion of a name and for http it is typically the "route name". So we either need a notion of sub-name or Activity.Name is actually a "component name". And we are building a spec for "component"

[tarekgh]

We are going to get rid of ActivitySource all together. I am going to share the new final proposal by tomorrow or so.

[SergeyKanzhelev]

Can we call ActivitySource Tracer in the new proposal?

[tarekgh]

We are going to get rid of ActivitySource all together.

[stephentoub]

This PR intended to share the proposal of the new APIs we need to add to System.Diagnostics namespace.

Since this won't be merged / hasn't been through API review / etc., a PR isn't necessary. Can you instead just link to the relevant commit/branch in your repo from the relevant issue? Thanks!

[stephentoub]

What does this mean for all activity use today where it's not being disposed? Are we introducing leaks?

[tarekgh]

We are not introducing a leak. This is added for simplifying the pattern of using Activity. I am going to add some code samples in the issue we'll use for the design review. here is some example quick example for what we currently have and what we are proposing:

// How Activity is used today:

var activityListener = new DiagnosticSource("Azure.Core.Http");

// Outer check to see if anyone is subscribed
if (activityListener.IsEnabled())
{
    Activity activity = null;
    // Check if anyone cares about activity
    if (activityListener.IsEnabled("Azure.Core.Http.Request"))
    {
        activity = new Activity("Azure.Core.Http.Request");
        activity.AddTag..
        activityListener.StartActivity(activity); // this does string concat and allocates every time
    }

    ...

    if (activity != null)
    {
        activityListener.StopActivity(activity);
    }
}

Here is what the new pattern will look like:

static ActivitySource source = new ActivitySource("Azure.Core.Http");

// ....

using (Activity activity = source.CraeteActivity())
{
        activity?.AddTag..
}

I am working to polish the sample code and add more samples.

[stephentoub]

HashCode.Combine?

[tarekgh]

good idea. the implementation here was just prototyping but it is useful to get such feedback anyway. Thanks.

[tarekgh]

I didn't use Hash.Combin because we need to compile for the full framework too and it is not worth it to add extra package dependency just for this method.

---

In reply to: 383035189 [](ancestors = 383035189)

[tarekgh]

@stephentoub

Since this won't be merged / hasn't been through API review / etc., a PR isn't necessary. Can you instead just link to the relevant commit/branch in your repo from the relevant issue? Thanks!

The intention so far is to get feedback from the experts on the proposed APIs before I go ahead and submit an official design review. Although this PR is not intended to be merged now, but I am expecting to enable it later when submitting the official implementation. do you mind keeping this PR for now?

[stephentoub]

do you mind keeping this PR for now?

Do you expect it'll be merged soon?

The intention so far is to get feedback from the experts on the proposed APIs before I go ahead and submit an official design review

That doesn't require a PR :-)

[noahfalk]

Glad to see this coming together!

[noahfalk]

If an app wrote @lmolkova's example I don't see any reason that it wouldn't function properly? The only downside of that pattern is the app author is paying the cost to create the ActivitySource, publish it, and have telemetry agents decide if they want to subscribe to it on each Activity generated.

Can we think about it from app dev convenience?

If the concern is being able to write simple one liners or not reading the docs then I'd guess the most likely risk is not calling Dispose() on the ActivitySource. For example:

    using(var a = new ActivitySource("foo").CreateActivity())
    {
    }

Looking at it a bit I believe we could eliminate the need for Dispose() by making a global list of weak GCHandles rather than a global list of strong ActivitySource references. We pay one additional pointer of memory per ActivitySource for the GCHandle and need a little more complicated implementation. ActivitySource is already around 100 bytes each (~4 pointers + 3 pointers for string + ~20*2 characters) so another pointer doesn't appear to be a significant relative overhead.

[noahfalk]

Making a note that we were going to look closer at whether this should be an IObservable pattern or stick with the ActiveList + event pattern.

If we do stick with list + event pattern this particular event signature+name feels unnecessarily abstract. This static event only gets called when a new ActivitySource is added to the list. We could change OperationEvent -> 'ActivitySourceCreated'. The ActivitySourceEventArgs includes the Operation field and the static type suggests it could have three different values. In practice the value would always be ActivitySourceCreated so the handler will never need to read it. We could eliminate the argument entirely or modify it to instead be a reference to the ActivitySource being added.

The current OperationEvent doesn't give a reference to the ActivitySource being added which means handlers would need to enumerate the ActiveList each time they receive the event. This makes adding N ActivitySources an O(N^2) operation which we probably wouldn't want. Keeping it O(N) requires that the event indicate which ActivitySource is being added.

[noahfalk]

I've been investigating this a bit more and had some thoughts:

1. The IObservable pattern doesn't work well here so I recommend we eliminate it from consideration. It forces the observer to cache every ActivitySource it subscribes to in order to unsubscribe later. That list wastes a bunch of memory and probably prevents any ActivitySource from ever getting collected. Technically the subscriber could use a second IObserver to iterate the list a second time to unsubscribe but it makes the code quite complex and I don't believe there is a way to be certain the unsubscribe pass is complete without making assumptions about threading mechanics and the BCL's implementation.

2. If we are moving to ActivityListener as an abstract base class then there is another option which seems appealing:

abstract class ActivityListener : IDisposable
{
    // Begins the flow of callback notifications
    public void Subscribe();

    // Unsubscribes from all activity notifications
    public void Dispose() { ... }

    // Derived implementations can override this to filter which activity names they will listen to.
    // Only names where this function returns true will ever trigger Sampling/Start/Stop callbacks
    // Internally the BCL would invoke this callback for all existing sources inside of Subscribe() and
    // again in the future whenever new sources are created. If the function returns true then the BCL
    // adds this listener to the source.
    public virtual bool ShouldListenToActivity(string name) { return true; }

    // activity callbacks
    public virtual bool ShouldCreateActivity(string name, ActivityTraceId id, ...) { return true; }
    public virtual void OnActivityStarted(Activity a) {}
    public virtual void OnActivityStopped(Activity a) {}
}

To use it the consuming code derives from the ActivityListener, instantiates an instance of the derived type and then calls Subscribe() to begin the flow of callbacks. This is very similar to the EventSource/EventListener pattern except I added an explicit Subscribe() step to ensure that events don't start showing up before the derived constructor has had a chance to run.

If we went this route we could eliminate these APIs because subscription is now automatic:
static ActivitySource.ActiveList
static ActivitySource.OperationEvent
ActivitySource.ActivityEvent

[tarekgh]

@noahfalk I have applied your suggestion. The only small change I did is I have moved the Subscribe method from the listener to ActivitySource and named it AddListener for the sake of discoverability. Let me know if you have any more feedback.

[tarekgh]

@stephentoub I'll close this PR in the next couple of days.

[tarekgh]

Closing this PR as I got the feedback and will try to open official PR when we finish the design review.