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

[WIP] Add semantic convention for specification 1.4.0 #1890

Conversation

tigrannajaryan
Copy link
Member

@tigrannajaryan tigrannajaryan commented May 7, 2021

[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

[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).
@tigrannajaryan
Copy link
Member Author

@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
Copy link

codecov bot commented May 7, 2021

Codecov Report

Merging #1890 (650c13c) into main (d20e722) will decrease coverage by 0.0%.
The diff coverage is 0.0%.

Impacted file tree graph

@@           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     
Impacted Files Coverage Δ
semconv/v1_4_0/http.go 0.0% <0.0%> (ø)
exporters/trace/jaeger/jaeger.go 93.4% <0.0%> (-1.1%) ⬇️
sdk/trace/batch_span_processor.go 87.1% <0.0%> (+1.5%) ⬆️

// patterns for OpenTelemetry things. This package aims to be the
// centralized place to interact with these conventions.
//
// Example usage:
Copy link
Member Author

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.

@Aneurysm9
Copy link
Member

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.

@tigrannajaryan
Copy link
Member Author

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.

@Aneurysm9
Copy link
Member

It looks like at least the attribute.Key constants will be interned. The attribute.Values created for enum values will likely still result in some duplicated struct instances, but they should at least share the string values they ultimately hold.

@tigrannajaryan
Copy link
Member Author

It looks like at least the attribute.Key constants will be interned. The attribute.Values created for enum values will likely still result in some duplicated struct instances, but they should at least share the string values they ultimately hold.

@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.

Copy link
Contributor

@MrAlias MrAlias left a 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.

Comment on lines +15 to +16
// Package semconv implements OpenTelemetry semantic conventions start from specification
// version 1.4.0 and until the next version that introduced any changes.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// 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.

Copy link
Member

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..."?

Copy link
Contributor

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.

@tigrannajaryan
Copy link
Member Author

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?

I think we don't because by definition semantic conventions may change and that means anyone who imports that semconv will break when the latest conventions are updated. So, I believe the safest approach is for every API user to explicitly import the right numbered semconv/<version> package and use that. Typically it will be the latest at the time when the code is written. The API user will be responsible for updating to a newer version of semconv/* and can do that at their leisure.

For historical reasons and to avoid breaking already existing code we can keep the semconv package and can make it an alias of semconv/1.4.0 (or whatever is the very first version we release).

@Aneurysm9
Copy link
Member

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?

I think we don't because by definition semantic conventions may change and that means anyone who imports that semconv will break when the latest conventions are updated. So, I believe the safest approach is for every API user to explicitly import the right numbered semconv/<version> package and use that.

Agreed. I initially thought that having semconv alias the latest version of the conventions would be nice, but as Tigran points out it could result in unexpected changes to attribute keys or values. I hope that the conventions quickly become stable enough that that isn't going to happen, but I still think it's best to leave that as a choice for the library and application authors.

@MrAlias
Copy link
Contributor

MrAlias commented May 13, 2021

SGTM. If we can get this out before our stable release I'm in favor of just moving the semconv package to be the latest and not aliasing. If not though, this sounds like a good approach.

@MadVikingGod
Copy link
Contributor

So we are going to pin /semconv to v1.4.0, because they might break things going forward? Wouldn't they be breaking their stability promises?

How does this work when 1.5.0 comes out? Do we have to manually find the difference and rearrange it?
When if ever do we update /semconv?

@Aneurysm9
Copy link
Member

So we are going to pin /semconv to v1.4.0, because they might break things going forward? Wouldn't they be breaking their stability promises?

How does this work when 1.5.0 comes out? Do we have to manually find the difference and rearrange it?
When if ever do we update /semconv?

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 semconv package entirely and only have semconv/v1_4_0 which would be imported as semconv wherever it was used. This should minimize the amount of work to update existing uses.

When v1.5.0 is released we would generate a new semconv/v1_5_0 package and ship that in a minor release. Consumers would be free to update their import references at their leisure.

@MadVikingGod
Copy link
Contributor

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 semconv within this codebase?

Currently, we use semconv in sdk/resouce, sdk/trace, zipkin exporter, jager exporter, and a number of internal/test packages. Would we fix these to the current version, or would they track the highest version?

@tigrannajaryan
Copy link
Member Author

So we are going to pin /semconv to v1.4.0, because they might break things going forward? Wouldn't they be breaking their stability promises?

The schemas concept specifically says that semantic conventions may change over time. It is not breaking a stability promise when conventions change.

@tigrannajaryan
Copy link
Member Author

Currently, we use semconv in sdk/resouce, sdk/trace, zipkin exporter, jager exporter, and a number of internal/test packages. Would we fix these to the current version, or would they track the highest version?

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. import semconv "go.opentelemetry.io/otel/semconv/v1_4_0" as shown in the example in this PR). If the code wants to become compliant with newer version of semantic conventions they need to manually update the import to point to the newer semconv package (e.g. import semconv "go.opentelemetry.io/otel/semconv/v1_5_0").

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.

@MrAlias
Copy link
Contributor

MrAlias commented Jun 10, 2021

Closing in favor of #1987

@MrAlias MrAlias closed this Jun 10, 2021
OpenTelemetry Go RC automation moved this from In progress to Done Jun 10, 2021
@tigrannajaryan
Copy link
Member Author

Thank you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants