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

Concern about the OpenTelemetry.SemanticConventions package #4086

Closed
reyang opened this issue Jan 19, 2023 · 5 comments
Closed

Concern about the OpenTelemetry.SemanticConventions package #4086

reyang opened this issue Jan 19, 2023 · 5 comments
Labels
bug Something isn't working

Comments

@reyang
Copy link
Member

reyang commented Jan 19, 2023

Bug Report

https://github.com/open-telemetry/opentelemetry-dotnet/tree/main/src/OpenTelemetry.SemanticConventions

  • This monolithic package contains things such as HTTP and Database - if the user is only using HTTP instrumentation, they will still have to include the extra DB related strings - which means extra size, and more importantly, it could trigger the static analysis false alerts.
  • It has a potential side effect of forcing the users to use the same version for HTTP and Database semantic conventions - need to understand why do we want to put such restriction (e.g. if application A takes dependency on library B, and both A/B want to report telemetry but using different versions of semantic conventions, would such scenario be blocked by the current approach?)
@martinjt
Copy link
Member

This is common across all the open telemetry language implementations, so remaining consistent with that approach should override the concerns here.

@Kielek
Copy link
Contributor

Kielek commented Aug 2, 2023

Putting content from #4612 and closing duplicated issue

Feature Request

Is your feature request related to a problem?

Current state of OpenTelemetry.SemanticConventions can leads to binary breaking changes in the minor rreleases (removal/changes of public const string/public static strings.
It can leads to runtime errorrs.

Describe the solution you'd like:

I think that the best option for this package will be to create nuget package which includes *.cs file with internal classes which can be included directly while compiling.
If each library have own copy, instrumentations based on different versions of semantic convection package can coexists.

Describe alternatives you've considered.

Fully deprecate this package: https://www.nuget.org/packages/OpenTelemetry.SemanticConventions

Additional Context

Follow up to open-telemetry/opentelemetry-dotnet-contrib#1100

@cijothomas
Copy link
Member

Removing Instrumentation-1.0.0 milestone. Instrumentations do not use this package, so addressing this should not block Instrumentation 1.0 release

@cijothomas cijothomas removed this from the Instrumentation-1.0.0 milestone Sep 11, 2023
@KalleOlaviNiemitalo
Copy link

SemanticConventions.cs.j2 currently generates public const string for attribute names and values, but public static readonly string for prefixes and events.

  • Removal of a public const string member typically does not cause an error at run time, because the C# compiler copies the referenced const values into the referencing assembly. Runtime errors are still possible if the members are accessed via reflection, though.

  • Removal of a public static readonly string member causes an error at run time if the removed member is referenced.

Another advantage of const is that constants can be referenced in C# switchcase statements.

As an application developer, I wanted to use the OpenTelemetry.SemanticConventions package to it make it easy for future maintainers to look up the official documentation of each attribute. The binary compatibility would not have been a problem because I can ensure that all application components reference consistent package versions. The greater problem is how out-of-date the package is; for example, it has "net.transport" rather than "network.transport".

@martinjt
Copy link
Member

Thanks @KalleOlaviNiemitalo.

We are working on a wider approach to the library that will allow for experimental and stable conventions to be represented, that's the reason this hasn't been updated.

I don't have an ETA, but I will make sure that these are noted.

I'm going to close off this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

5 participants