-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
[extension/ballast] Remove the deprecated memory ballast extension #10671
[extension/ballast] Remove the deprecated memory ballast extension #10671
Conversation
9552534
to
2e8f3bd
Compare
Co-authored-by: Curtis Robert <[email protected]>
…lmuth/opentelemetry-collector into remove-ballast-extension
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #10671 +/- ##
==========================================
+ Coverage 92.22% 92.23% +0.01%
==========================================
Files 409 406 -3
Lines 19134 19101 -33
==========================================
- Hits 17646 17618 -28
+ Misses 1127 1123 -4
+ Partials 361 360 -1 ☔ View full report in Codecov by Sentry. |
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.
Can we possibly split this process in more steps?
For example, a first step before outright removal can be:
- Removing it from the Helm chart
- Removing support from it in the service (that is, fixing Change
process_runtime_heap_alloc_bytes
metric to include memory ballast size #8342) and documenting this - Removing any mention of it on docs and examples on the opentelemetry org
Then a second step can be removing it from the official binaries while keeping a source-only release, and a final step would be removing the source code entirely.
I think this would solve our immediate issue and make the deprecation process more manageable (not all users would get this removed at the same time, so support requests are more easy to manage).
I know this is more annoying but
- this extension has been here since forever
- we don't know which users have been listening to our deprecations
so I think we need to be gentle and divide this into steps
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
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.
Have we seen any users complaining after open-telemetry/opentelemetry-collector-releases/pull/607? I see #10995, have we had any other reports?
If #10995 is the only one, I am comfortable merging this
So far I've only seen #10995 |
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.
Oki, then I would propose we wait until, say, Wednesday next week and merge if we don't get more reports
Description
This PR removes the deprecated memory ballast extension and all the logic in place in memorylimiter and service that was using it.
Link to tracking issue
Related to #8343. I don't want to close it until the helm chart is updated.
Testing
Unit tests