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

Track JVM gc time in histogram #2903

Merged
merged 13 commits into from
Nov 15, 2022
Merged

Conversation

jack-berg
Copy link
Member

Fixes #2902.

cc @open-telemetry/java-instrumentation-maintainers, @open-telemetry/java-maintainers

jack-berg and others added 2 commits October 31, 2022 14:46
…-metrics.md

Co-authored-by: Trask Stalnaker <trask.stalnaker@gmail.com>
Copy link
Member

@bogdandrutu bogdandrutu left a comment

Choose a reason for hiding this comment

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

Why do you think that a default of histogram is better?

@jack-berg
Copy link
Member Author

Why do you think that a default of histogram is better?

@bogdandrutu not sure I understand. Better than what?

In general, histogram is a good fit for this because it includes a lot of useful data for analysis:

  • You can use the sum to analyze the total time spent in gc
  • Can look at the count to analyze the number of gc events
  • Can look at the max to analyze the maximum time spent in any single gc event (i.e. what was my longest "stop the world" gc event?)
  • Can look at the distribution to analyze the distribution of time of gc events (i.e. how long do typical gc events take? how long do outlier gc events take?)

@trask
Copy link
Member

trask commented Nov 8, 2022

@bogdandrutu just wanted to make sure you don't have any significant concerns before we request merging, thx!

@jack-berg
Copy link
Member Author

FYI I've updated the metric name to process.runtime.jvm.gc.duration, prompted by this comment. I quite like @trask's suggestion to use the duration suffix instead of time. Makes it clear that this metric measures the distribution of individual gc event durations, rather than the total time spent in in garbage collection.

Please take another look approvers @breed-splk, @mateuszrzeszutek @jmacd @trask

@trask
Copy link
Member

trask commented Nov 15, 2022

@bogdandrutu @jmacd @reyang @jsuereth - the Java folks would like to merge this if there's no further concerns

@reyang reyang merged commit a6888fc into open-telemetry:main Nov 15, 2022
@reyang
Copy link
Member

reyang commented Nov 15, 2022

There is a wider discussion brought up by @jsuereth this morning regarding whether we should have different way of exposing metrics for the same concept or not. We should continue that discussion in the semantic convention workgroup, no need to block this PR since it has strong support from Java folks.

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.

Track JVM gc time
8 participants