-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
[WIP] Add semantic convention for specification 1.4.0 #1890
[WIP] Add semantic convention for specification 1.4.0 #1890
Conversation
[This is a draft. Do not merge.] I suggest to use separate packages for each version of the semantic conventions that has an changes since the last defined package. The semantic convention package includes semantic convention definitions, most of which will typically be aliases of semantic conventions of the last defined package, and perhaps include some changed semantic convention definitions, plus the schema URL constant that matches the version of the semantic conventions. Individual instrumentation libraries are supposed to import and use one and only one semantic convention package (although different libraries used in one application may import different semantic convention packages).
@Aneurysm9 @MrAlias please have a look. This is one possible way to tied the semantic conventions and the schema URL together. I wonder if there is a better way though, that is less line count. |
Codecov Report
@@ Coverage Diff @@
## main #1890 +/- ##
=======================================
- Coverage 79.2% 79.1% -0.1%
=======================================
Files 139 140 +1
Lines 7459 7466 +7
=======================================
Hits 5908 5908
- Misses 1304 1312 +8
+ Partials 247 246 -1
|
// patterns for OpenTelemetry things. This package aims to be the | ||
// centralized place to interact with these conventions. | ||
// | ||
// Example usage: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Aneurysm9 @MrAlias see this example.
I've just created #1891 to generate most of the semconv package from the spec YAML. I wonder if we should just use the generator and keep each version-named package self-contained rather than referring back to the "current" semconv package. |
@Aneurysm9 This is a good suggestion, I like it much better than what I did in this PR. One small concern I have is creating lots of duplicate strings, one set per each version of semconv, where the vast majority of the strings are the same across versions. I don't know if Go complier does string interning and eliminates duplicates. If it does then this is not a problem at all. |
It looks like at least the |
@Aneurysm9 sounds good, thanks for checking. Let's go with your proposal. Once #1891 is merged let's decide how we want to name semconv packages so that we have one per version and whether we want to automate it further or just require manual re-generation every time a new spec version is released. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we should just use the generator and keep each version-named package self-contained rather than referring back to the "current" semconv package.
I would prefer this approach as well.
Do we want the semconv
package to then be a mirror of the latest versioned package? Like if semconv/v1.0.4
is the latest semconv
would import it and export things similar, but in reverse, of this PR?
Once #1891 is merged let's decide how we want to name semconv packages so that we have one per version and whether we want to automate it further or just require manual re-generation every time a new spec version is released.
#1891 is merged (yay! 🎉). I'm in favor of full automation (i.e. GitHub action) here, including warnings to not submit changes to the files that are not from the automation.
// Package semconv implements OpenTelemetry semantic conventions start from specification | ||
// version 1.4.0 and until the next version that introduced any changes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// Package semconv implements OpenTelemetry semantic conventions start from specification | |
// version 1.4.0 and until the next version that introduced any changes. | |
// Package semconv/v1_4_0 implements v1.4.0 of the OpenTelemetry semantic conventions. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is that valid? Or should it be "Package v1_4_0 implements..."?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not entirely sure. I was going off this https://pkg.go.dev/golang.org/x/sys/unix
The sys/unix package provides...
I'm go with the other way as well.
I think we don't because by definition semantic conventions may change and that means anyone who imports that For historical reasons and to avoid breaking already existing code we can keep the |
Agreed. I initially thought that having |
SGTM. If we can get this out before our stable release I'm in favor of just moving the |
So we are going to pin How does this work when 1.5.0 comes out? Do we have to manually find the difference and rearrange it? |
I think the ideal case is that spec v1.4.0 is released with the schema specification before we release 1.0. In that case we eliminate the existing When v1.5.0 is released we would generate a new |
Ok, I understand that model, and I think that's an ok idea. Would that mean this PR would be changed to mostly be a move of everything to the new subdir? How do we manage the Currently, we use |
The schemas concept specifically says that semantic conventions may change over time. It is not breaking a stability promise when conventions change. |
I suggest the following: every piece of external code that uses the Otel Go API and needs to refer to semantic conventions should import a specific version of semconv package (e.g. We can have a "highest" semconv package which aliases the latest generated semconv package, but I suggest that we make the "highest" an internal package in SDK and do not make it available in the API. So "highest" at one point in time may be a copy/alias of "go.opentelemetry.io/otel/semconv/v1_4_0", and later when a new version is released "highest" may be updated to become an alias of "go.opentelemetry.io/otel/semconv/v1_5_0". The package name for "highest" can be something like "go.opentelemetry.io/otel/internal/semconv". The SDK code itself can import that internal "highest" semconv package. If the conventions change such that the "highest" semconv package has a breaking change then it is SDK maintainer's responsibility to fix the SDK code that imports it. |
Closing in favor of #1987 |
Thank you. |
[This is a draft. Do not merge.]
I suggest to use separate packages for each version of the semantic conventions
that has an changes since the last defined package.
The semantic convention package includes semantic convention definitions, most of
which will typically be aliases of semantic conventions of the last defined package,
and perhaps include some changed semantic convention definitions, plus the schema URL
constant that matches the version of the semantic conventions.
Individual instrumentation libraries are supposed to import and use one and only one
semantic convention package (although different libraries used in one application
may import different semantic convention packages).
Continuation of #1889