-
Notifications
You must be signed in to change notification settings - Fork 5
Adds Experimental Attributes #43
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
base: main
Are you sure you want to change the base?
Changes from 7 commits
bcba74b
f537ffc
dbaf8c2
edd0b20
d2f4a25
0a1b9fb
f70018c
30eb17f
aadf494
14b8b8a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -70,6 +70,12 @@ logger[metadataKey: .init(name: OTelAttribute.http.request.method)] = "POST" | |
| logger[metadataKey: .init(name: OTelAttribute.http.response.statusCode)] = "200" | ||
| ``` | ||
|
|
||
| ### Unstable Instrumentations | ||
|
|
||
| [Unstable](https://opentelemetry.io/docs/specs/otel/versioning-and-stability/#semantic-conventions-stability) attributes are available, but are gated behind a non-default `Experimental` trait. To use them, you must explicitly include the this trait in your `Package.swift`. | ||
|
|
||
| **Be aware that unstable attributes may experience breaking changes on minor version updates of this package, so use with caution!** If this breakage is unacceptable, but non-standard tags are still required, a version dependency range that only allows patch updates should be used. | ||
|
||
|
|
||
| ## Contributing | ||
|
|
||
| ### Generation | ||
|
|
||
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 created the experimental trait as discussed in #27, but what do you think about using
"Unstable"instead?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.
Thanks, I'll get to a detailed review later. One thing I thought about is that just having the trait without additional markers on the unstable attributes may be a bit too little because you don't immediately see whether you'd even need the trait based on the attributes used. One option I thought about is to prefix them all with experimental/unstable, so it's immediately obvious from the call-side. What do you think?
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 like that. Plus we should add a clear note in the Readme explaining that if you use the experimental trait, you must use
.upToNextMinoras they might break between minor versions.Uh oh!
There was an error while loading. Please reload this page.
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.
Yeah, definitely a good concern. The only thing I'd want to be careful of is to abide by the
Developmentsection of the stability guide, in particular:I believe that this would disallow any symbol-prefixing or namespacing (since
abcbecoming stable would transition the symbol fromexperimentalAbc->abc, and break existing users).Two approaches that come to mind and are compatible with the spec:
@availableattributes to provide compiler warnings on non-stable attributes. This may be too intrusive, as some projects consider any compiler warnings to be a code smell.What do you guys think?
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.
Maybe doc comments are enough, combined with the package trait.
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.
Ah, thanks for linking to the spec. I agree that it makes sense to follow it here. As you suggested option 1 with
@availablechecks seems too intrusive because it would basically "punish" users of the unstable API always, not just when upgrading to a release that graduated some of the unstable attributes.So overall I agree that the best option is to make it very clear in the documentation of individual attributes and in the trait documentation.
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.
Sounds good! I've added a big, bolded
**UNSTABLE**to the beginning of each attribute documentation here: aadf494