-
Notifications
You must be signed in to change notification settings - Fork 901
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
Specify ...jvm.gc.duration buckets using advice API #8436
Specify ...jvm.gc.duration buckets using advice API #8436
Conversation
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.
LGTM.
.../library/src/main/java/io/opentelemetry/instrumentation/runtimemetrics/GarbageCollector.java
Show resolved
Hide resolved
.../library/src/main/java/io/opentelemetry/instrumentation/runtimemetrics/GarbageCollector.java
Show resolved
Hide resolved
aa25995
to
02c8d9e
Compare
} | ||
((ExtendedDoubleHistogramBuilder) builder) | ||
.setAdvice(advice -> advice.setExplicitBucketBoundaries(emptyList())); |
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.
Hi @mateuszrzeszutek, sorry for tagging in an old PR, but I am just wondering the reason for making explicitBucketBoundaries as an empty list?
Before version 1.26.0 I am pretty sure it used to have the explicitBucketBoundaries.
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.
Hey @kes2464 ,
We've had many discussions on what the buckets for the gc metrics should be; and for a long while we were of the opinion that we should not provide any, and instead allow the users to set the buckets they want via the SDK Views. The current implementation reflects that.
We'll likely add some buckets when the JVM metrics become stable in the 2.0 version, see open-telemetry/semantic-conventions#317
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.
Thanks for the detailed explanation @mateuszrzeszutek, I think it makes sense for GC metrics as it may vary depending on the apps and so on. For me, as long as they can be created it's fine, and adding a new View worked perfectly.
This needs to be merged after #8334