Skip to content
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/ballastextension] Deprecate memory_ballast extension #8803

Merged

Conversation

mx-psi
Copy link
Member

@mx-psi mx-psi commented Nov 6, 2023

Description:

Based on user reports on open-telemetry/opentelemetry-helm-charts/issues/891 and the discussion on #8343, we can deprecate the memory ballast extension in favor of using GOMEMLIMIT. This PR:

  • Deprecates the memory ballast extension in the README
  • Removes references to the memory ballast extension on docs
  • Updates k8s example to use GOMEMLIMIT with the same approach as in the Helm chart (80% of memory limit)
  • Deprecates the memory ballast extension Go module

Once this PR is accepted, open-telemetry/opentelemetry-helm-charts/issues/891 can move ahead with enabling useGOMEMLIMIT by default on the Helm chart.

Other issues will be opened for opentelemetry.io, the Opentelemetry Operator and other parts of the OpenTelemetry project to remove references to this extension once the PR is merged.

No explicit timeline is given for removal of the extension.

Link to tracking Issue: Updates #8343

Copy link

codecov bot commented Nov 6, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (034e427) 91.47% compared to head (d973e54) 91.47%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #8803   +/-   ##
=======================================
  Coverage   91.47%   91.47%           
=======================================
  Files         320      320           
  Lines       17187    17187           
=======================================
  Hits        15722    15722           
  Misses       1165     1165           
  Partials      300      300           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mx-psi
Copy link
Member Author

mx-psi commented Nov 6, 2023

@dmitryax @Aneurysm9 @TylerHelmuth @JaredTan95

Copy link
Contributor

@dloucasfx dloucasfx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @mx-psi for pushing this and getting us closer to GOMEMLIMIT !

Not sure if this was already discussed, the ballast extension has been used in conjunction with the memory-limiter processor to control GC;

With GOMEMLIMIT do we need to change the behavior of memory-limiter ?
The memory-limiter currently provides 2 main tasks:

  • signal the pipeline to drop or stop accepting new data
  • call GC explicitly

With GOMEMLIMIT we should no longer worry about explicitly calling GC anymore, however, we might still need to keep the drop or stop accepting data?
I also understand that @dmitryax suggested to move the memory-limiter as a receiver helper.

@bogdandrutu
Copy link
Member

@mx-psi would be good to look into the memory limiter as well.

@mx-psi
Copy link
Member Author

mx-psi commented Nov 7, 2023

@bogdandrutu @dloucasfx There are a couple dependencies on the memory ballast in other parts of the Collector:

IMO, we should first

  1. deprecate the memory ballast
  2. update all our documentation and examples to stop mentioning the memory ballast
  3. update the Helm chart, operator and Docker default configuration to use GOMEMLIMIT
  4. (can happen in parallel with 2 & 3) wait for a couple of releases

After these three happen, we can think about changing the behavior of other parts of the Collector that depend on the memory ballast, including the memory limiter (if the receiver part) and the metrics. Does that make sense? Do you think there is specific parts we should do now as part of this PR?

Copy link
Contributor

This PR was marked stale due to lack of activity. It will be closed in 14 days.

@github-actions github-actions bot added the Stale label Nov 22, 2023
@mx-psi
Copy link
Member Author

mx-psi commented Nov 22, 2023

@bogdandrutu @dloucasfx can you take another look?

@github-actions github-actions bot removed the Stale label Nov 23, 2023
Copy link
Contributor

github-actions bot commented Dec 7, 2023

This PR was marked stale due to lack of activity. It will be closed in 14 days.

@github-actions github-actions bot added the Stale label Dec 7, 2023
@mx-psi
Copy link
Member Author

mx-psi commented Dec 7, 2023

Looks like we are first going to enable GOMEMLIMIT by default in the Helm Chart for a couple of versions before doing this, you can follow along at open-telemetry/opentelemetry-helm-charts/issues/891

@github-actions github-actions bot removed the Stale label Dec 8, 2023
@bogdandrutu bogdandrutu merged commit b04b551 into open-telemetry:main Dec 19, 2023
32 checks passed
@github-actions github-actions bot added this to the next release milestone Dec 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants