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

[Feature request]: Support instrumentation scope attributes on ActivitySource #93795

Closed
Tracked by #97522
lmolkova opened this issue Oct 20, 2023 · 9 comments · Fixed by #102678
Closed
Tracked by #97522

[Feature request]: Support instrumentation scope attributes on ActivitySource #93795

lmolkova opened this issue Oct 20, 2023 · 9 comments · Fixed by #102678
Assignees
Labels
api-approved API was approved in API review, it can be implemented api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Diagnostics.Activity blocking Marks issues that we want to fast track in order to unblock other important work in-pr There is an active PR which will close this issue when it is merged
Milestone

Comments

@lmolkova
Copy link

lmolkova commented Oct 20, 2023

When creating a tracer (ActivitySource) users should be able to specify a set of instrumentation scope attributes - https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/api.md#get-a-tracer.
See open-telemetry/opentelemetry-dotnet#4316 for the context.

Instrumentation scope attributes can be used to reduce instrumentation overhead and reduce the over-the-wire size of attributes.

For example, Azure SDK provides Azure resource provider namespace is used by Azure Monitor (and can be used by other backends) to show icons, find corresponding resources in Azure, and allow navigating to them, etc. this value is constant for a given tracer and should be passed as an instr scope attribute rather than span attribute.

.NET does not allow to provide such attributes as of today.

proposal added by @tarekgh

Proposal

namespace System.Diagnostics;

    public sealed class ActivitySource : IDisposable
    {
        public ActivitySource(string name, string? version = "") { ... }
+       public ActivitySource(string name, string? version, IEnumerable<KeyValuePair<string, object?>>? tags) { ... }

+       public IEnumerable<KeyValuePair<string, object?>>? Tags { get; }
    }

Alternative Proposal

namespace System.Diagnostics;

    public class ActivitySourceOptions
    {
        public ActivitySourceOptions(string name) {    }

        public string Name { get; set; }
        public string? Version { get; set; }
        public IEnumerable<KeyValuePair<string, object?>>? Tags { get; set; }
    }

    public sealed class ActivitySource : IDisposable
    {
        ...
        public ActivitySource(ActivitySourceOptions options) { ... }
        
        public IEnumerable<KeyValuePair<string, object?>>? Tags { get; }
    }
@dotnet-issue-labeler dotnet-issue-labeler bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Oct 20, 2023
@ghost ghost added the untriaged New issue has not been triaged by the area owner label Oct 20, 2023
@ghost
Copy link

ghost commented Oct 23, 2023

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

Issue Details

See open-telemetry/opentelemetry-dotnet#2417

When an instrumentation library emits telemetry, it usually follows specific semantic conventions. E.g. HTTP traces should follow Http semconv

However, OTel has yet to declare its first semantic conventions as stable and they change a lot.
Even after stability is declared, new attributes can be added or altered in a non-breaking manner. In the worst case, OTel may introduce breaking changes to semconv and bump a major version.

So, it's important to know which version of OTel semconv instrumentation emits for the consumers of such telemetry.
From otel perspective, instrumentations that don't populate SchemaUrl can't change at all until they start to emit SchemaUrl.

In the absence of SchemaUrl Azure SDKs in .NET populate it as an attribute on all spans using its own custom convention and increasing telemetry size with redundant information (SchemaUrl is passed as part of instrumentation scope, which is sent over the wire per batch of spans).

In addition to SchemaUrl, there are other parameters instrumentations may provide when obtaining tracers(ActivitySources) or meters - https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/api.md#get-a-tracer.

See open-telemetry/opentelemetry-dotnet#4316 for the context.

Instrumentation scope attributes can be used to reduce instrumentation overhead and reduce the over-the-wire size of attributes.

Author: lmolkova
Assignees: -
Labels:

area-System.Diagnostics.Tracing, untriaged, needs-area-label

Milestone: -

@tommcdon tommcdon added enhancement Product code improvement that does NOT require public API changes/additions and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners untriaged New issue has not been triaged by the area owner labels Oct 23, 2023
@tommcdon tommcdon added this to the 9.0.0 milestone Oct 23, 2023
@tommcdon
Copy link
Member

Assigning to @noahfalk to help coordinate

@tarekgh
Copy link
Member

tarekgh commented Oct 23, 2023

This is a duplicate of #63651. @lmolkova can we close this and continue the discussion on the other one?

CC @cijothomas @reyang

@ghost
Copy link

ghost commented Oct 23, 2023

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

Issue Details

See open-telemetry/opentelemetry-dotnet#2417

When an instrumentation library emits telemetry, it usually follows specific semantic conventions. E.g. HTTP traces should follow Http semconv

However, OTel has yet to declare its first semantic conventions as stable and they change a lot.
Even after stability is declared, new attributes can be added or altered in a non-breaking manner. In the worst case, OTel may introduce breaking changes to semconv and bump a major version.

So, it's important to know which version of OTel semconv instrumentation emits for the consumers of such telemetry.
From otel perspective, instrumentations that don't populate SchemaUrl can't change at all until they start to emit SchemaUrl.

In the absence of SchemaUrl Azure SDKs in .NET populate it as an attribute on all spans using its own custom convention and increasing telemetry size with redundant information (SchemaUrl is passed as part of instrumentation scope, which is sent over the wire per batch of spans).

In addition to SchemaUrl, there are other parameters instrumentations may provide when obtaining tracers(ActivitySources) or meters - https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/api.md#get-a-tracer.

See open-telemetry/opentelemetry-dotnet#4316 for the context.

Instrumentation scope attributes can be used to reduce instrumentation overhead and reduce the over-the-wire size of attributes.

Author: lmolkova
Assignees: noahfalk
Labels:

enhancement, area-System.Diagnostics.Tracing, area-System.Diagnostics.Activity

Milestone: 9.0.0

@tarekgh tarekgh added api-suggestion Early API idea and discussion, it is NOT ready for implementation and removed area-System.Diagnostics.Tracing enhancement Product code improvement that does NOT require public API changes/additions labels Oct 23, 2023
@lmolkova
Copy link
Author

thanks @tarekgh and sorry I did not find the original issue. The original one still does not cover the instrumentation scope attributes, so I'd prefer to keep it open. I'll modify the description to scope the issue to them.

@lmolkova lmolkova changed the title [Feature request]: Support SchemaUrl and instrumentation scope attributes for OpenTelemetry tracing and metrics [Feature request]: Support instrumentation scope attributes for OpenTelemetry tracing and metrics Oct 23, 2023
@tarekgh
Copy link
Member

tarekgh commented Oct 23, 2023

Thanks @lmolkova for clarification. We already done #84321 in .NET 8.0. Is that what you are asking for? or you want to have the same for ActivitySource too?

@lmolkova
Copy link
Author

Cool! Yes, I mean ActivitySource, great to know it's addressed on meters.

@lmolkova lmolkova changed the title [Feature request]: Support instrumentation scope attributes for OpenTelemetry tracing and metrics [Feature request]: Support instrumentation scope attributes on ActivitySource Oct 23, 2023
@noahfalk noahfalk assigned tarekgh and unassigned noahfalk Oct 25, 2023
@tarekgh
Copy link
Member

tarekgh commented Oct 25, 2023

open-telemetry/opentelemetry-dotnet#3636

@tarekgh tarekgh added the blocking Marks issues that we want to fast track in order to unblock other important work label May 11, 2024
@terrajobst
Copy link
Member

terrajobst commented May 21, 2024

Video

  • We should make it simpler
    • Have one method with just the required parameter (name)
    • Have one method that takes all the optional parameters
    • We can't remove methods, but we can hide them and remove all optionality to avoid ambiguity
namespace System.Diagnostics;

public partial class ActivitySource
{
    // Existing, left as-is
    // public ActivitySource(string name);

    // Existing, remove optionality and mark EB never
    [EditorBrowsable(EditorBrowsableState.Never)]
    public ActivitySource(string name, string? version);

    // New
    public ActivitySource(string name, string? version = "", IEnumerable<KeyValuePair<string, object?>>? tags = default);

    public IEnumerable<KeyValuePair<string, object?>>? Tags { get; }
}

@terrajobst terrajobst 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 labels May 21, 2024
@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 May 24, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Jun 29, 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 api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Diagnostics.Activity blocking Marks issues that we want to fast track in order to unblock other important work 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