Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[API Proposal]: Activity.IsStopped (functionality required to conform to OpenTelemetry specification) #63353

Closed
Tracked by #62027
Oberon00 opened this issue Jan 4, 2022 · 13 comments · Fixed by #63713
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-System.Diagnostics.Activity
Milestone

Comments

@Oberon00
Copy link

Oberon00 commented Jan 4, 2022

Background and motivation

As far as I know, the Activity class is meant to provide all the functionality required by the OpenTelemetry tracing specification for the concept that is called "Span" in OTel terms.

However, I could not find a way to get the following from Activity:

It must also be able to reliably determine whether the Span has ended (some languages might implement this by having an end timestamp of null, others might have an explicit hasEnded boolean).

-- https://github.com/open-telemetry/opentelemetry-specification/blob/v1.8.0/specification/trace/sdk.md#additional-span-interfaces

At first, I thought one could check if Activity.Duration != TimeSpan.Zero, but while this is indeed true for all stopped/ended activities, even activities that are still running can have a non-zero duration if SetEndTime was called but Stop/Dispose was not.

In some situations, one could could use the callback from the Source and maintain a ConditionalWeakTable to record whether an Activity has stopped, but this is rather cumbersome, slow and not possible if you do not have control over the activities' source.

It seems that Activity already has this implemented as private functinality as a property IsFinished: https://github.com/dotnet/runtime/blob/v6.0.1/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Activity.cs#L1301-L1303

Exposing the existing name IsFinished would also be an acceptable IMHO (I think the most OTel-like name would be HasEnded, but since End is called Stop, my primary suggestion is IsStopped).

The interface in OpenTelemetry Java is here https://github.com/open-telemetry/opentelemetry-java/blob/v1.9.1/sdk/trace/src/main/java/io/opentelemetry/sdk/trace/ReadableSpan.java#L64-L71 and here https://github.com/open-telemetry/opentelemetry-java/blob/v1.9.1/sdk/trace/src/main/java/io/opentelemetry/sdk/trace/data/SpanData.java#L117-L122

API Proposal

namespace System.Diagnostics
{
    public class Activity: IDisposable
    {
        // ...
        public bool IsStopped { get; }
        // ...
    }
}

API Usage

var a = new Activity("foo");
Debug.Assert(!a.IsStopped);
a.Start();
Debug.Assert(!a.IsStopped);
a.Stop();
Debug.Assert(a.IsStopped);

In an ActivityListener.ActivityStopped callback, IsStopped of the notifying Activity must also be true to conform to OTel semantics.

Alternative Designs

If SetEndTime would always unconditionally Stop the activity, the check for nonzero Duration could be used instead of using a separate property. However, that would probably be an unacceptable breaking change. Note: The XML doc of Duration claims that calling SetEndTime ends the activity (is ending the same as stopping?) but unless Duration != Zero does logically end the activity (without invoking a listener), this does not seem to be true.

Alternative Workaround

OpenTelemetry SDK is already listening to Activity Start and Stop events, it can manually either create a weak reference table mapping the Activity object to its wrapper span object and then mark the span ended when receiving Stop event. Another idea is to set some property/tag value on the Activity when receiving the stop event. These are not clean solutions considering the simplicity we'll get if we added the proposed API.

Risks

None known. This proposal would add a new property exposing existing functionality that is required as an API surface by the OpenTelemetry specification.

@Oberon00 Oberon00 added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Jan 4, 2022
@dotnet-issue-labeler dotnet-issue-labeler bot added area-System.Diagnostics.Tracing untriaged New issue has not been triaged by the area owner labels Jan 4, 2022
@ghost
Copy link

ghost commented Jan 4, 2022

Tagging subscribers to this area: @tarekgh, @tommcdon, @pjanotti, @safern
See info in area-owners.md if you want to be subscribed.

Issue Details

Background and motivation

As far as I know, the Activity class is meant to provide all the functionality required by the OpenTelemetry tracing specification for the concept that is called "Span" in OTel terms.

However, I could not find a way to get the following from Activity:

It must also be able to reliably determine whether the Span has ended (some languages might implement this by having an end timestamp of null, others might have an explicit hasEnded boolean).

-- https://github.com/open-telemetry/opentelemetry-specification/blob/v1.8.0/specification/trace/sdk.md#additional-span-interfaces

At first, I thought one could check if Activity.Duration != TimeSpan.Zero, but while this is indeed true for all stopped/ended activities, even activities that are still running can have a non-zero duration if SetEndTime was called but Stop/Dispose was not.

In some situations, one could could use the callback from the Source and maintain a ConditionalWeakTable to record whether an Activity has stopped, but this is rather cumbersome, slow and not possible if you do not have control over the activities' source.

It seems that Activity already has this implemented as private functinality as a property IsFinished: https://github.com/dotnet/runtime/blob/v6.0.1/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Activity.cs#L1301-L1303

Exposing the existing name IsFinished would also be an acceptable IMHO (I think the most OTel-like name would be HasEnded, but since End is called Stop, my primary suggestion is IsStopped).

The interface in OpenTelemetry Java is here https://github.com/open-telemetry/opentelemetry-java/blob/v1.9.1/sdk/trace/src/main/java/io/opentelemetry/sdk/trace/ReadableSpan.java#L64-L71 and here https://github.com/open-telemetry/opentelemetry-java/blob/v1.9.1/sdk/trace/src/main/java/io/opentelemetry/sdk/trace/data/SpanData.java#L117-L122

CC @open-telemetry/dotnet-approvers

API Proposal

namespace System.Diagnostics.Generic
{
    public class Activity: IDisposable
    {
        // ...
        public bool IsStopped { get; }
        // ...
    }
}

API Usage

var a = new Activity("foo");
Debug.Assert(!a.IsStopped);
a.Start();
Debug.Assert(!a.IsStopped);
a.Stop();
Debug.Assert(a.IsStopped);

In an ActivityListener.ActivityStopped callback, IsStopped of the notifying Activity must also be true to conform to OTel semantics.

Alternative Designs

If SetEndTime would always unconditionally Stop the activity, the check for nonzero Duration could be used instead of using a separate property. However, that would probably be an unacceptable breaking change.

Risks

None known. This proposal would add a new property exposing existing functionality that is required as an API surface by the OpenTelemetry specification.

Author: Oberon00
Assignees: -
Labels:

api-suggestion, area-System.Diagnostics.Tracing, untriaged

Milestone: -

@tarekgh tarekgh removed the untriaged New issue has not been triaged by the area owner label Jan 4, 2022
@tarekgh tarekgh added this to the 7.0.0 milestone Jan 4, 2022
@tarekgh
Copy link
Member

tarekgh commented Jan 4, 2022

The proposal looks reasonable to me.

CC @noahfalk @cijothomas @reyang

@noahfalk
Copy link
Member

noahfalk commented Jan 4, 2022

Yeah, this looks fine to me too.

@reyang
Copy link

reyang commented Jan 5, 2022

Is this considered part of the API or SDK? (this is part of the SDK spec).

@tarekgh
Copy link
Member

tarekgh commented Jan 5, 2022

Looks it is part of the SDK but how the SDK can implement that without having Activity provide this functionality?

@ghost
Copy link

ghost commented Jan 5, 2022

Tagging subscribers to this area: @dotnet/area-system-diagnostics-activity
See info in area-owners.md if you want to be subscribed.

Issue Details

Background and motivation

As far as I know, the Activity class is meant to provide all the functionality required by the OpenTelemetry tracing specification for the concept that is called "Span" in OTel terms.

However, I could not find a way to get the following from Activity:

It must also be able to reliably determine whether the Span has ended (some languages might implement this by having an end timestamp of null, others might have an explicit hasEnded boolean).

-- https://github.com/open-telemetry/opentelemetry-specification/blob/v1.8.0/specification/trace/sdk.md#additional-span-interfaces

At first, I thought one could check if Activity.Duration != TimeSpan.Zero, but while this is indeed true for all stopped/ended activities, even activities that are still running can have a non-zero duration if SetEndTime was called but Stop/Dispose was not.

In some situations, one could could use the callback from the Source and maintain a ConditionalWeakTable to record whether an Activity has stopped, but this is rather cumbersome, slow and not possible if you do not have control over the activities' source.

It seems that Activity already has this implemented as private functinality as a property IsFinished: https://github.com/dotnet/runtime/blob/v6.0.1/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Activity.cs#L1301-L1303

Exposing the existing name IsFinished would also be an acceptable IMHO (I think the most OTel-like name would be HasEnded, but since End is called Stop, my primary suggestion is IsStopped).

The interface in OpenTelemetry Java is here https://github.com/open-telemetry/opentelemetry-java/blob/v1.9.1/sdk/trace/src/main/java/io/opentelemetry/sdk/trace/ReadableSpan.java#L64-L71 and here https://github.com/open-telemetry/opentelemetry-java/blob/v1.9.1/sdk/trace/src/main/java/io/opentelemetry/sdk/trace/data/SpanData.java#L117-L122

API Proposal

namespace System.Diagnostics.Generic
{
    public class Activity: IDisposable
    {
        // ...
        public bool IsStopped { get; }
        // ...
    }
}

API Usage

var a = new Activity("foo");
Debug.Assert(!a.IsStopped);
a.Start();
Debug.Assert(!a.IsStopped);
a.Stop();
Debug.Assert(a.IsStopped);

In an ActivityListener.ActivityStopped callback, IsStopped of the notifying Activity must also be true to conform to OTel semantics.

Alternative Designs

If SetEndTime would always unconditionally Stop the activity, the check for nonzero Duration could be used instead of using a separate property. However, that would probably be an unacceptable breaking change. Note: The XML doc of Duration claims that calling SetEndTime ends the activity (is ending the same as stopping?) but unless Duration != Zero does logically end the activity (without invoking a listener), this does not seem to be true.

Risks

None known. This proposal would add a new property exposing existing functionality that is required as an API surface by the OpenTelemetry specification.

Author: Oberon00
Assignees: -
Labels:

api-suggestion, area-System.Diagnostics.Activity

Milestone: 7.0.0

@reyang
Copy link

reyang commented Jan 5, 2022

Looks it is part of the SDK but how the SDK can implement that without having Activity provide this functionality?

What I can think of (not necessarily against the proposal, just to call out that there might be many different solutions):

  1. The SDK has full access to start/stop event, so it could do something.
  2. The Activity API can expose something totally different (e.g. the end timestamp) and still allow the SDK to tell if an activity is stopped.

@Oberon00
Copy link
Author

Oberon00 commented Jan 5, 2022

The end timestamp is already exposed, but can be set without stopping the Activity (see the issue description).

The SDK might be able to solve this with events, but it then SHOULD ensure that users of the SDK have a way to tell if a Activity they received in SpanProcessor.OnStart is stopped at any later point in time (https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/sdk.md#onstart "It SHOULD be possible to keep a reference to this span object and updates to the span SHOULD be reflected in it."). As stated in the issue description, this would probably require a ConditionalWeakTable.

@tarekgh
Copy link
Member

tarekgh commented Jan 5, 2022

"It SHOULD be possible to keep a reference to this span object and updates to the span SHOULD be reflected in it."). As stated in the issue description, this would probably require a ConditionalWeakTable.

Do we have any other scenarios that need to keep the references of the span objects? If not, then it is not worth it to introduce that just for checking spans is stopped or not.

The other idea could be to set some tag or custom property on the Activity when receiving the stop event and use it when you want to check. I am still thinking exposing Activity.IsStopped would be a cleaner solution here.

@tarekgh
Copy link
Member

tarekgh commented Jan 8, 2022

If there is no more feedback, can we move forward with the proposal? @reyang any more feedback? Thanks!

@reyang
Copy link

reyang commented Jan 8, 2022

@reyang any more feedback?

Nope. I guess it is possible to implement this efficiently in the SDK without having to introduce a new API to the runtime. But I don't see adding a new runtime API as bad idea if there is good reason and use case.

@tarekgh
Copy link
Member

tarekgh commented Jan 8, 2022

@Oberon00 I have added a small section Alternative Workaround in the description to capture @reyang feedback. Feel free to edit it as needed.

@tarekgh tarekgh added api-ready-for-review API is ready for review, it is NOT ready for implementation blocking Marks issues that we want to fast track in order to unblock other important work and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation labels Jan 8, 2022
@bartonjs
Copy link
Member

bartonjs commented Jan 11, 2022

Video

Looks good as proposed

namespace System.Diagnostics
{
    public partial class Activity
    {
        public bool IsStopped { get; }
    }
}

@bartonjs bartonjs added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for review, it is NOT ready for implementation blocking Marks issues that we want to fast track in order to unblock other important work labels Jan 11, 2022
@tarekgh tarekgh self-assigned this Jan 12, 2022
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Jan 12, 2022
ML, Extensions, Globalization, etc, POD. automation moved this from 7.0.0 to Done Jan 14, 2022
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Jan 14, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Feb 13, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-System.Diagnostics.Activity
Development

Successfully merging a pull request may close this issue.

5 participants