- 
                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?
Adds Experimental Attributes #43
Conversation
        
          
                Package.swift
              
                Outdated
          
        
      | traits: [ | ||
| .trait(name: "Tracing"), | ||
| .default(enabledTraits: ["Tracing"]), | ||
| .trait(name: "Experimental"), | 
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 .upToNextMinor as they might break between minor versions.
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 Development section of the stability guide, in particular:
OpenTelemetry clients MUST NOT be designed in a manner that breaks existing users when a signal transitions from Development to Stable. This would punish users of the release candidate, and hinder adoption.
I believe that this would disallow any symbol-prefixing or namespacing (since abc becoming stable would transition the symbol from experimentalAbc -> abc, and break existing users).
Two approaches that come to mind and are compatible with the spec:
- Using @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.
- Adjusting documentation to more obviously flag non-stable attributes as mentioned in the issue. This may not be intrusive enough, as a user might not view the docs before using.
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 @available checks 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
        
          
                README.md
              
                Outdated
          
        
      |  | ||
| [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. | 
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.
Let's add an explicit instruction to use .upToNextMinor, so that at least we promise not to break them between patch versions.
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.
Good thought - I was going to do that, but it looks like .upToNextMinor is deprecated, so I went with explaining the equivalent range restriction. Is that right that it's deprecated?
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 think this is the right one, on Range, not deprecated https://developer.apple.com/documentation/swift/range/uptonextminor(from:)
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.
Oh awesome, thanks for linking that! I've adjusted the documentation in 30eb17f to recommend that upToNextMinor is used.
| 
 Edit: This has been reverted to  | 
c8b909c    to
    14b8b8a      
    Compare
  
    
This generates and exposes unstable attributes like those marked
development,experimentalorrelease_candidate, and gates them behind a non-defaultExperimentaltrait.closes #27