-
Notifications
You must be signed in to change notification settings - Fork 896
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
Modify Exporter format variables for 1.0 (allowing further specification post 1.0) #1358
Modify Exporter format variables for 1.0 (allowing further specification post 1.0) #1358
Conversation
Can we define environment variables for protocols (both OTLP and Zipkin) together with possible values for those variables, but NOT require SDKs to implement ALL of them before 1.0? |
@iNikem I think for OTLP yes. For Zipkin, there was some concern over what things meant. I think it'd be fine to open a PR following this one where that can be discussed, but the idea is to allow it to be specified after the 1.0 next week, in the event the discussion doesn't finish in time for 1.0. |
@jsuereth is this PR ready for review? |
yes! Sorry, didn't mean to leave WIP on it for that long. |
I am concerned that if SIGs can choose their own default protocols then, after we update the spec, SIGs will have to change their defaults, which will be a breaking change. What am I missing? |
@!iNIkem we're not going to specify the default. If you want to force a protocol, you'll need to use the variable or configuration. |
specification/protocol/exporter.md
Outdated
- `OTEL_EXPORTER_OTLP_TRACE_PROTOCOL` | ||
- `OTEL_EXPORTER_OTLP_METRIC_PROTOCOL` | ||
|
||
SDKs are free to choose a default, if no configuration is provided. |
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.
@jsuereth the issue seems to be this. If sdks can pick a default, they can end up with different defaults. The intent of the spec has generally been to have shared behavior outside of api idioms. For something completely contained in the SDK it could still be ok, but protocol leaks out. For example, if given an app written in lang1 and another in lang2, we would expect both to connect to the same collector with default settings. But if the collector only enables proto, and lang2 only supports json, the behavior of the defaults changes drastically.
No snark intended, if even JS, the most json-oriented language around, can support gRPC, I'm hoping we could converge on it being a required default to prevent surprises. Having no default, rather than an SDK-specific default, therefore requiring explicit user configuration seems fine though.
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 agree that this isn't an ideal situation. This is called "limiting what we do for 1.0, and providing a stable release". We'll improve over time. When we're ready to make a breaking change (and understand this problem better) we could specify a default.
The proposal here is to unblock 1.0 and allow something better to come later.
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.
To be clear: the plan is to improve this done by the time we do a 'full' release along with stable Metrics (or maybe even sooner). Hopefully we will triage the remaining bits accordingly ;)
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.
@jsuereth if we say that SDK should not have a default and require this? It will be annoying for the users while we fix this, but this will make us fix it sooner than later and we have all the freedom?
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 suggest calling this an "unspecified default".
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.
@bogdandrutu Yes. Right now we don't know what protocols are supported across languages well enough to specifiy a default without severely risking the 1.0 release.
The idea here is we write down what we have, making room for later fixes. We will NOT break users, so yes this limits how we operate, and when a change to this specification could occur. The idea is: Ship a stable 1.0 and iterate on improvements instead of continually delaying.
I just want to make sure it's clear that I don't really like this solution, but this is the kind of thing we'll need to get used to if we want to offer compatibility + stability guarantees, with timely releases. We can't have a 1.0 release anytime soon AND fix this issue, so let's just give ourselves enough room to do something better for users in the future. I think it's impossible for us to have found every possible usability bug of this sort prior to 1.0. Hoping this will set a precedent for what we do with issues like this post-1.0 in the land of "keep things stable AND improve".
@jmacd +1 to that suggestion, added.
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.
Ping @bogdandrutu
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.
Ok, hopefully everyone understands this way :) Right now it is required for user to specify the values and SDK will not work without a value specified :) otherwise they actually define a default.
discussed at the sig mtg today, and it is desired to have a resolution merged for GA so applying labels to this PR |
@open-telemetry/specs-approvers Please review/approve this PR ;) |
specification/protocol/exporter.md
Outdated
- `OTEL_EXPORTER_OTLP_TRACE_PROTOCOL` | ||
- `OTEL_EXPORTER_OTLP_METRIC_PROTOCOL` | ||
|
||
SDKs are free to choose a default, if no configuration is provided. |
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 suggest calling this an "unspecified default".
cbc9d63
to
eba50fd
Compare
@open-telemetry/specs-approvers Please review (or approve) this one ;) |
While I understand the pressure to release something, I still feel like this is brushing under the rug too much. We've identified a point that could create compatibility problems - if we ignore it, we'll have even more to deal with than the issues we didn't think about in advance which are definitely going to appear! Rather than converging on an ambiguous spec, no spec should be better right? These options come to mind
And btw if the intent of this change is along these lines, than I think it's not explicit enough - it should make clear what we want the languages to commit to in terms of messaging about their stability guarantees. |
I have nothing to say about @anuraaga concerns, but I disagree with your options :) I think we have to declare env variable stable. |
Do you think this means prioritizing stabilizing them over consistency among languages which is questionable with unspecified defaults? If we're good with it then that's fine too (it's been so hard to get divergence among languages of API concerns didn't think env vars would be so easy ;) |
Yes, I think stable 1.0 which includes env variables is more important that consistency of unspecified behaviour across languages :) |
@jsuereth please rebase |
- Ensure environment variables have consistent names - Update documentation to leave format UNSPECIFIED in 1.0 with an allowance to add config later.
Co-authored-by: Anuraag Agrawal <anuraaga@gmail.com>
Co-authored-by: Bartlomiej Obecny <bobecny@gmail.com>
eba50fd
to
c17e324
Compare
This is only as effective as users reading the docs. In my experience, users will ignore all "warnings" if they have a strong need for something and then be upset with you when you break it. We only have so much friction we can cause and this doesn't seem like a good use of social capital.
This hasn't been my experience. Also, I don't think anything is locked down such that it can't be slowly evolved/migrated. We should be avoiding unnecessary churn (e.g. ENV variable renames) post 1.0 because it just causes friction and unrest for users. If we really want this project to be stable and widely adopted, then we need to make sure users are succesful and not surprised. Changing environment variables is probably one of the MOST surprising things, especially if you switch from something on-default to something default.
Suggestions welcome. The intent here was to specify a minimum (what we felt was already common) so SDKs won't diverge past that minimum, but not spin cycles gaining consensus on the full specification. Regarding whether ENVIRONMENT variables are considered in Semantic Conventions + versioning, I think that belongs in a different PR than this, but I'm happy to take a stab at it. |
@bogdandrutu it's rebased now. |
We have enough approvals. Let's merge this by the end of the day 😃 |
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.
Please add an entry to CHANGELOG.md
. Thanks!
the future: | ||
|
||
- `OTEL_EXPORTER_OTLP_PROTOCOL` | ||
- `OTEL_EXPORTER_OTLP_TRACE_PROTOCOL` |
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.
Should this be OTEL_EXPORTER_OTLP_TRACES_PROTOCOL
?
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.
Yes please :)
Merging to expedite things. Will cook a follow-up PR myself. |
…ion post 1.0) (open-telemetry#1358) * OAdapt open-telemetry#1340 for 1.0 of Trace. - Ensure environment variables have consistent names - Update documentation to leave format UNSPECIFIED in 1.0 with an allowance to add config later.
Adapts #1340
Changes
Related issues #1336