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

[improve][monitor] Rename "GC Pauses" to "GC Time" in Pulsar JVM dashboards #18891

Merged
merged 1 commit into from
Dec 16, 2022

Conversation

lhotari
Copy link
Member

@lhotari lhotari commented Dec 12, 2022

Motivation

Modifications

Replace "GC Pauses" with "GC Time" in Grafana JVM dashboards.

Documentation

  • doc
  • doc-required
  • doc-not-needed
  • doc-complete

- source metric is not about GC pauses.
- the source metric is "jvm_gc_collection_seconds_sum" which is not the same as "GC pause"
- The metric is defined in Prometheus Java client
  - "Time spent in a given JVM garbage collector in seconds."
  - source code is https://github.com/prometheus/client_java/blob/e68daf23336eb5de7856df406eb1d497f51ad3be/simpleclient_hotspot/src/main/java/io/prometheus/client/hotspot/GarbageCollectorExports.java#L53
    - calls GarbageCollectorMXBean.getCollectionTime()
      - Javadoc https://docs.oracle.com/en/java/javase/11/docs/api/java.management/java/lang/management/GarbageCollectorMXBean.html#getCollectionTime()
        "Returns the approximate accumulated collection elapsed time in milliseconds."
     - Stackoverflow Q&A: https://stackoverflow.com/a/44686539
@lhotari lhotari added this to the 2.12.0 milestone Dec 12, 2022
@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Dec 12, 2022
@lhotari
Copy link
Member Author

lhotari commented Dec 12, 2022

There's also a metric for GC Pauses in Pulsar. The metric name is jvm_full_gc_pause.
Here's the implementation:

metrics.put("jvm_full_gc_pause", currentFullGcTime);
metrics.put("jvm_full_gc_count", currentFullGcCount);

It uses total safepoint time metric under the covers,

It's a bit misleading to call it "full gc" since it's more than full gc. More details in https://blanco.io/blog/jvm-safepoint-pauses/

@codecov-commenter
Copy link

codecov-commenter commented Dec 12, 2022

Codecov Report

Merging #18891 (61c209c) into master (3180a4a) will decrease coverage by 2.85%.
The diff coverage is 33.33%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #18891      +/-   ##
============================================
- Coverage     46.17%   43.31%   -2.86%     
+ Complexity    10359     8404    -1955     
============================================
  Files           703      597     -106     
  Lines         68845    56863   -11982     
  Branches       7382     5902    -1480     
============================================
- Hits          31788    24632    -7156     
+ Misses        33448    29422    -4026     
+ Partials       3609     2809     -800     
Flag Coverage Δ
unittests 43.31% <33.33%> (-2.86%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
.../main/java/org/apache/pulsar/PulsarStandalone.java 0.00% <0.00%> (ø)
...broker/intercept/ManagedLedgerInterceptorImpl.java 0.00% <0.00%> (ø)
...va/org/apache/pulsar/client/impl/ConsumerImpl.java 15.09% <0.00%> (-0.04%) ⬇️
...he/pulsar/client/impl/MultiTopicsConsumerImpl.java 22.78% <0.00%> (-0.09%) ⬇️
...nsaction/pendingack/impl/PendingAckHandleImpl.java 36.61% <50.00%> (-15.23%) ⬇️
.../pulsar/broker/service/AbstractBaseDispatcher.java 49.71% <66.66%> (-7.76%) ⬇️
...sistent/PersistentDispatcherMultipleConsumers.java 47.56% <100.00%> (-4.17%) ⬇️
...g/apache/pulsar/client/impl/RawBatchConverter.java 3.12% <0.00%> (-89.07%) ⬇️
...g/apache/pulsar/broker/service/StreamingStats.java 0.00% <0.00%> (-83.79%) ⬇️
...sar/broker/stats/metrics/ManagedLedgerMetrics.java 23.88% <0.00%> (-76.12%) ⬇️
... and 183 more

Copy link
Member

@tisonkun tisonkun left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

@lhotari lhotari merged commit 56ad217 into apache:master Dec 16, 2022
lifepuzzlefun pushed a commit to lifepuzzlefun/pulsar that referenced this pull request Jan 10, 2023
pgier added a commit to datastax/kaap that referenced this pull request Jun 25, 2024
Change jvm dashboard labels to be more accurate.
See also: apache/pulsar#18891
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc-not-needed Your PR changes do not impact docs ready-to-test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants