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

Support adding links to Activity after it is created #97680

Closed
Tracked by #97522
vishweshbankwar opened this issue Jan 30, 2024 · 11 comments · Fixed by #101381
Closed
Tracked by #97522

Support adding links to Activity after it is created #97680

vishweshbankwar opened this issue Jan 30, 2024 · 11 comments · Fixed by #101381
Labels
api-approved API was approved in API review, it can be implemented area-System.Diagnostics.Activity in-pr There is an active PR which will close this issue when it is merged
Milestone

Comments

@vishweshbankwar
Copy link
Contributor

vishweshbankwar commented Jan 30, 2024

OpenTelemetry has added spec for supporting link addition to spans after the span is created. PR marking spec as stable - open-telemetry/opentelemetry-specification#3887.

This is currently not possible in .NET. Links can only be added when the activity is being created see this

Opening the issue for tracking as supporting this new feature would require addition of new API in System.Diagnostics.DiagnosticSource.

API Proposal

namespace System.Diagnostics
{
    public partial class Activity : IDisposable
    {
        public System.Diagnostics.Activity AddLink(System.Diagnostics.ActivityLink link);
    }
}
@ghost ghost added the untriaged New issue has not been triaged by the area owner label Jan 30, 2024
@ghost
Copy link

ghost commented Jan 30, 2024

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

Issue Details

OpenTelemetry has added experimental spec for supporting link addition to spans after the span is created. This is currently not possible in .NET. Links can only be added when the activity is being created see this

opening the issue for tracking as supporting this new feature would require addition of new API in System.Diagnostics.DiagnosticSource.

Author: vishweshbankwar
Assignees: -
Labels:

area-System.Diagnostics.Tracing

Milestone: -

@ghost
Copy link

ghost commented Jan 30, 2024

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

Issue Details

OpenTelemetry has added experimental spec for supporting link addition to spans after the span is created. This is currently not possible in .NET. Links can only be added when the activity is being created see this

opening the issue for tracking as supporting this new feature would require addition of new API in System.Diagnostics.DiagnosticSource.

Author: vishweshbankwar
Assignees: -
Labels:

untriaged, area-System.Diagnostics.Activity

Milestone: -

@tarekgh tarekgh added this to the 9.0.0 milestone Jan 30, 2024
@ghost ghost removed the untriaged New issue has not been triaged by the area owner label Jan 30, 2024
@tarekgh
Copy link
Member

tarekgh commented Jan 30, 2024

@vishweshbankwar I have marked this for future release till we get the OTel stable and then we can bring it back to current release as needed.

@tarekgh tarekgh modified the milestones: 9.0.0, Future Jan 30, 2024
@hahn-kev
Copy link

hahn-kev commented Feb 1, 2024

I'm also interested in this, there's a library that I'd like to enrich an activity with a link but I can't since it's already been created.

@martinjt
Copy link

@tarekgh this has now been marked stable in the spec.

open-telemetry/opentelemetry-specification#3887

@tarekgh
Copy link
Member

tarekgh commented Mar 27, 2024

https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/api.md#link

@vishweshbankwar could you please add the API proposal on the top? I guess this will be a simple like Activity.AddLink(ActivityLike link)

@vishweshbankwar
Copy link
Contributor Author

https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/api.md#link

@vishweshbankwar could you please add the API proposal on the top? I guess this will be a simple like Activity.AddLink(ActivityLike link)

@tarekgh Added.

@antonfirsov
Copy link
Member

System.Diagnostics.Activity AddLink(System.Diagnostics.ActivityLink link);

Do we really need AddLink to create & return an activity? Would this cover all activity creation concerns? ActivitySource.CreateActivity has multiple overloads with many args.

@vishweshbankwar
Copy link
Contributor Author

System.Diagnostics.Activity AddLink(System.Diagnostics.ActivityLink link);

Do we really need AddLink to create & return an activity? Would this cover all activity creation concerns? ActivitySource.CreateActivity has multiple overloads with many args.

@antonfirsov - The proposal is consistent with how the current Add* extensions work

e.g. https://learn.microsoft.com/en-us/dotnet/api/system.diagnostics.activity.addevent?view=net-8.0

@antonfirsov
Copy link
Member

Ok, missed that Activity has fluent APIs.

Anything preventing us from marking this api-ready-for-review? It is a prerequisite for an optimal implementation of #93832, so it would be nice to have a review scheduled soonish.

@dotnet-policy-service dotnet-policy-service bot added the in-pr There is an active PR which will close this issue when it is merged label Apr 22, 2024
@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 labels Apr 22, 2024
@bartonjs
Copy link
Member

bartonjs commented Apr 23, 2024

Video

  • We generally don't do the fluent pattern, but that's consistent with the type already.
  • So, looks good as proposed.
namespace System.Diagnostics
{
    public partial class Activity
    {
        public System.Diagnostics.Activity AddLink(System.Diagnostics.ActivityLink link);
    }
}

@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 Apr 23, 2024
@github-actions github-actions bot locked and limited conversation to collaborators May 27, 2024
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 in-pr There is an active PR which will close this issue when it is merged
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants