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

[adservice] add adServiceManualGC featureflag #1463

Merged
merged 9 commits into from
Mar 25, 2024

Conversation

EislM0203
Copy link
Contributor

@EislM0203 EislM0203 commented Mar 15, 2024

Changes

This PR introduces a new feature flag, adServiceManualGC, for the ad service. When activated, the ad service periodically fills up its heap and then performs full garbage collections. This is an interesting, real world observability scenario because it simulates memory problems in the application which afaik no feature flag does this way. Metrics that monitor heap and memory usage are automatically captured by the Java agent. Looking forward to your feedback!

Merge Requirements

For new features contributions please make sure you have completed the following
essential items:

  • CHANGELOG.md updated to document new feature additions
  • Appropriate documentation updates in the docs
  • Appropriate Helm chart updates in the helm-charts

Maintainers will not merge until the above have been completed. If you're unsure
which docs need to be changed ping the
@open-telemetry/demo-approvers.

@EislM0203 EislM0203 marked this pull request as ready for review March 15, 2024 10:57
@EislM0203 EislM0203 requested a review from a team March 15, 2024 10:57
@puckpuck
Copy link
Contributor

This is a great idea!

I question adding the memory metrics manually when they are already captured automatically by the Java agent. jvm_memory_used_bytes is the metric you are looking for, and it will have labels for memory type (heap, non-heap) as well as pool type. We have this for every Java-based service.

@EislM0203
Copy link
Contributor Author

This is a great idea!

I question adding the memory metrics manually when they are already captured automatically by the Java agent. jvm_memory_used_bytes is the metric you are looking for, and it will have labels for memory type (heap, non-heap) as well as pool type. We have this for every Java-based service.

@puckpuck, thanks a lot for that info, totally missed that. Removed the custom metrics with my latest commit!

@puckpuck
Copy link
Contributor

@EislM0203 it seems like the Memory utils logic got added back into your branch. Can you clean that part up properly, please?

@EislM0203
Copy link
Contributor Author

@EislM0203 it seems like the Memory utils logic got added back into your branch. Can you clean that part up properly, please?

Hey @puckpuck, what do you mean? As previously discussed I only removed the custom metrics. This does not mean I can get rid of the MemoryUtils class as it provides the GarbageCollectionTrigger class with the needed JVM heap related utility methods.

@puckpuck puckpuck added the helm-update-required Requires an update to the Helm chart when released label Mar 20, 2024
@puckpuck puckpuck added the docs-update-required Requires documentation update label Mar 21, 2024
@puckpuck
Copy link
Contributor

I tested this, and it works as expected. Doesn't trigger any error state per say, unless # of GC counts is something that you cared about. I think it makes sense to have a Demo JVM or Demo Runtime dashboard to showcase the various metrics we get from the JVM and other runtimes. Such a dashboard could be used to understand the impact of this feature flag.

@julianocosta89 julianocosta89 merged commit 5c6b801 into open-telemetry:main Mar 25, 2024
25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs-update-required Requires documentation update helm-update-required Requires an update to the Helm chart when released
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants