[chloggen] Add new change_type value for library API changes #360
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #360 +/- ##
==========================================
- Coverage 60.70% 60.66% -0.04%
==========================================
Files 49 49
Lines 2298 2301 +3
==========================================
+ Hits 1395 1396 +1
- Misses 754 756 +2
Partials 149 149
☔ View full report in Codecov by Sentry. |
| NewComponent = "new_component" | ||
| Enhancement = "enhancement" | ||
| BugFix = "bug_fix" | ||
| LibraryAPIChange = "library_api_change" |
There was a problem hiding this comment.
As I mentioned in open-telemetry/opentelemetry-collector-contrib#24014, this new type can be confusing for the go instrumentation library. All the changes are library changes. If we "properly" follow this new type, we combine all of them under one section.
Even for the collector, we need to have a way to differentiate different types of "Go API only" changes. We have a lot of them in core.
Can we maybe have another orthogonal input like a boolean library_change? That can be used to generate two sections for library and user-facing changes, each section will have all the types if applicable, like:
## 1.0 User-facing changes
### 🛑 Breaking changes 🛑
foo
bar
### 🚩 Deprecations 🚩
foo
bar
### 🚀 New components 🚀
new component
### 🧰 Bug fixes 🧰
foo
bar
foobar
## 1.0 Library changes
### 🛑 Breaking changes 🛑
foo
bar
### 🚩 Deprecations 🚩
foo
bar
### 🚀 New components 🚀
new component
### 🧰 Bug fixes 🧰
foo
bar
foobar
There was a problem hiding this comment.
Can we maybe have another orthogonal input like a boolean library_change?
I agree this is an orthogonal concern, but library api changes are not mutually exclusive with user-facing changes. For example, when adding an option to a configuration we must make an API change as well. So while I agree at a high level, I propose we should have an orthogonal input that supports the three cases of user-only, api-only, or both.
Additionally, in the interest of keeping this tool somewhat generic, I suggest that the input be a slice where the possible values are enumerated (possibly as part of the configuration of the tool). e.g.
# only relevant to users
audience: [ user ]
# only relevant to api users
audience: [ api ]
# relevant to both
audience: [ user, api ] # make it optional and use this as the default?There was a problem hiding this comment.
This sounds good to me 👍
And I can imagine even more options. E.g., if we change mdatagen to generate new internal code, users and api are not affected but users of mdatagen command outside of the repo might need to change a lot in their codebase to adapt to the new generated code. Maybe that can be another option like tooling. In that case, audience probably is not the best word, maybe affects?
We also need to define how this will be generated. Will we have one section per option and duplicate the entries touching more than one "audience"? In that case, it's probably better to have separate changelog files.
There was a problem hiding this comment.
this new type can be confusing for the go instrumentation library
This is not meant to be used for the Go instrumentation library, or even for the Collector core I'd say, only for Collector contrib. I think any change we make, including audience is going to have that problem: what is the distinction between user and api on opentelemetry-go?
Even for the collector, we need to have a way to differentiate different types of "Go API only" changes. We have a lot of them in core.
Issue open-telemetry/opentelemetry-collector-contrib#24014 was meant to be about contrib only. If you want to include core, I feel like the discussion is very different.
I agree this is an orthogonal concern, but library api changes are not mutually exclusive with user-facing changes. For example, when adding an option to a configuration we must make an API change as well. So while I agree at a high level, I propose we should have an orthogonal input that supports the three cases of user-only, api-only, or both.
What kind of changes would be user-only? AIUI, any user-facing change is also a library change, since it changes the behavior of at least one symbol.
And I can imagine even more options. E.g., if we change mdatagen to generate new internal code, users and api are not affected but users of mdatagen command outside of the repo might need to change a lot in their codebase to adapt to the new generated code. Maybe that can be another option like tooling. In that case, audience probably is not the best word, maybe affects?
Isn't that just a change to the mdatagen component? Why would it need to be different?
General comment: while I am okay with the changes proposed above, I think they are very different from what I had in mind with Option B on open-telemetry/opentelemetry-collector-contrib/issues/24014 and I would prefer if either of you opens a PR with a concrete proposal in that line and we can close this one if your PR ends up being the accepted solution.
There was a problem hiding this comment.
What kind of changes would be user-only? AIUI, any user-facing change is also a library change, since it changes the behavior of at least one symbol.
Bug fixes often involve no changes to anything exported by the library. Performance enhancements can be made with no api changes. I agree deprecations cannot be only user facing though.
Breaking changes may need some further consideration but if the purpose of noting api-only changes is to inform developers that their code will break, there may be an argument to be made that mutating telemetry is breaking to the user but not necessarily to the developer who integrates with the library api.
There was a problem hiding this comment.
Bug fixes often involve no changes to anything exported by the library. Performance enhancements can be made with no api changes. I agree deprecations cannot be only user facing though.
Right, they are a library change but not a library API change. I was thinking about library changes in general, where I'd say any user-facing change is also a change of this kind, but I agree that the API itself (what symbols exist and what their names and types/signatures are) does not change in those cases.
Is it just a different implementation than you had in mind? Option B was "Have a separate changelog section for Go API only changes", which I think is the goal of either approach.
I see #360 (comment) as Option A, these are two separate changelogs, even if on the same file. Sorry, I feel like the original issue was not clear enough as to what each option entailed.
There was a problem hiding this comment.
Right, they are a library change but not a library API change. I was thinking about library changes in general, where I'd say any user-facing change is also a change of this kind, but I agree that the API itself (what symbols exist and what their names and types/signatures are) does not change in those cases.
What's the case for documenting library changes that are not library API changes? In my mind, a library change that is not a library API change is unremarkable except in the way that it impacts users.
There was a problem hiding this comment.
I see #360 (comment) as Option A, these are two separate changelogs, even if on the same file. Sorry, I feel like the original issue was not clear enough as to what each option entailed.
I see what you are saying. I didn't appreciate the difference between Options A and B, except the one file vs two aspect. It seems I am suggesting Option A is necessary, though I think having a single file is better.
There was a problem hiding this comment.
What's the case for documenting library changes that are not library API changes? In my mind, a library change that is not a library API change is unremarkable except in the way that it impacts users.
I don't think these are important for opentelemetry-collector-contrib, but I guess I was thinking about other users of the tool (opentelemetry-go) where we do care about library changes. But I guess if we are making something specific to contrib this is not important.
I see what you are saying. I didn't appreciate the difference between Options A and B, except the one file vs two aspect. It seems I am suggesting Option A is necessary, though I think having a single file is better.
Should we go back to the original issue to decide? I am not sure how to move this forward
There was a problem hiding this comment.
Should we go back to the original issue to decide? I am not sure how to move this forward
I've proposed a new approach in #364 that would support Option A. Let's continue discussion in the original issue.
|
Sounds like the majority opinion on open-telemetry/opentelemetry-collector-contrib#24014 moved towards Option A, so I am closing this and we can continue over at #364 |
Updates open-telemetry/opentelemetry-collector-contrib/issues/24014.
This adds a new
library_api_changetype that would apply to Go API only changes in the OpenTelemetry Collector contrib repository (e.g. those related to open-telemetry/opentelemetry-collector-contrib/issues/17273).