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

Register all available Go runtime metrics #2005

Merged
merged 1 commit into from
Feb 1, 2023

Conversation

andreasgerstmayr
Copy link
Contributor

What this PR does:
Registers all available Go runtime metrics

Which issue(s) this PR fixes:
PR #1528 upgraded prometheus/client_golang to version v1.12.2, which contains the following change:

Reverting addition of new ~80 runtime metrics by default. You can enable this back with GoRuntimeMetricsCollection option or GoRuntimeMemStatsCollection | GoRuntimeMetricsCollection for smooth transition.

The removed metrics contain some interesting GC metrics (e.g. go_gc_cycles_total_gc_cycles_total). This PR (re-)introduces these metrics:

# TYPE go_cgo_go_to_c_calls_calls_total counter
# TYPE go_gc_cycles_automatic_gc_cycles_total counter
# TYPE go_gc_cycles_forced_gc_cycles_total counter
# TYPE go_gc_cycles_total_gc_cycles_total counter
# TYPE go_gc_heap_allocs_by_size_bytes histogram
# TYPE go_gc_heap_allocs_bytes_total counter
# TYPE go_gc_heap_allocs_objects_total counter
# TYPE go_gc_heap_frees_by_size_bytes histogram
# TYPE go_gc_heap_frees_bytes_total counter
# TYPE go_gc_heap_frees_objects_total counter
# TYPE go_gc_heap_goal_bytes gauge
# TYPE go_gc_heap_objects_objects gauge
# TYPE go_gc_heap_tiny_allocs_objects_total counter
# TYPE go_gc_limiter_last_enabled_gc_cycle gauge
# TYPE go_gc_pauses_seconds histogram
# TYPE go_gc_stack_starting_size_bytes gauge
# TYPE go_memory_classes_heap_free_bytes gauge
# TYPE go_memory_classes_heap_objects_bytes gauge
# TYPE go_memory_classes_heap_released_bytes gauge
# TYPE go_memory_classes_heap_stacks_bytes gauge
# TYPE go_memory_classes_heap_unused_bytes gauge
# TYPE go_memory_classes_metadata_mcache_free_bytes gauge
# TYPE go_memory_classes_metadata_mcache_inuse_bytes gauge
# TYPE go_memory_classes_metadata_mspan_free_bytes gauge
# TYPE go_memory_classes_metadata_mspan_inuse_bytes gauge
# TYPE go_memory_classes_metadata_other_bytes gauge
# TYPE go_memory_classes_os_stacks_bytes gauge
# TYPE go_memory_classes_other_bytes gauge
# TYPE go_memory_classes_profiling_buckets_bytes gauge
# TYPE go_memory_classes_total_bytes gauge
# TYPE go_sched_gomaxprocs_threads gauge
# TYPE go_sched_goroutines_goroutines gauge
# TYPE go_sched_latencies_seconds histogram

No currently existing metrics are removed.
This PR is based on grafana/loki#6962.

Alternatively we could also make this configurable if we don't want to export all metrics by default, wdyt?

Checklist

  • Tests updated
  • Documentation added
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

@CLAassistant
Copy link

CLAassistant commented Jan 18, 2023

CLA assistant check
All committers have signed the CLA.

Copy link
Member

@joe-elliott joe-elliott left a comment

Choose a reason for hiding this comment

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

Thanks for the addition! I like the idea of adding these metrics, but my impression is that they are rarely used and can be high cardinality.

Can we add a flag to turn these on and have them off by default?

@andreasgerstmayr
Copy link
Contributor Author

Thanks for the review!

I've moved them to a feature flag now (off by default). I wasn't sure if I should name it enable-go-runtime-metrics, go-runtime-metrics.enabled or more verbose enable-all-go-runtime-metrics. The former sounds more natural for this feature, while the other form is used for components (auth.enabled, search.enabled). Let me know if I should change it, or move the config setting to some other component.

Copy link
Member

@joe-elliott joe-elliott left a comment

Choose a reason for hiding this comment

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

I'm good on the name in the PR. It looks like you have a conflict to resolve?

Heads up that we are going to hold on merging this til after 2.0 is cut. We will likely get an RC out this week and start aggressively working on 2.0. We will revisit merging this likely in a few weeks.

@andreasgerstmayr
Copy link
Contributor Author

I've rebased the PR and fixed the conflict.

Ok, np - thanks for letting me know. Looking forward to Tempo 2.0 :)

Signed-off-by: Andreas Gerstmayr <agerstmayr@redhat.com>
Copy link
Member

@joe-elliott joe-elliott left a comment

Choose a reason for hiding this comment

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

Thanks for the improvement!

@joe-elliott joe-elliott merged commit 05bbe06 into grafana:main Feb 1, 2023
@andreasgerstmayr
Copy link
Contributor Author

Thanks for merging & congrats on the v2 release yesterday! 🎉

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.

3 participants