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

Expose runtime Go metrics #3535

Merged
merged 4 commits into from
Feb 12, 2024
Merged

Expose runtime Go metrics #3535

merged 4 commits into from
Feb 12, 2024

Conversation

joanlopez
Copy link
Contributor

What?

It exposes the /metrics endpoint when --profiling-enabled, with the Go runtime metrics from https://github.com/prometheus/client_golang.

Why?

As stated in #2955, these metrics can be critical to spot problems and further optimize scripts, extensions, and k6 core. It comes almost by free (as part of github.com/prometheus/client_golang is already an -indirect- dependency of the project), and in the future we could even introduce our own custom metrics, like the amount of VUs, ongoing HTTP requests, etc.

Checklist

  • I have performed a self-review of my code.
  • I have added tests for my changes.
  • I have run linter locally (make lint) and all checks pass.
  • I have run tests locally (make tests) and all tests pass.
  • I have commented on my code, particularly in hard-to-understand areas.

Related PR(s)/Issue(s)

Closes #2955

@joanlopez joanlopez requested a review from dgzlopes January 11, 2024 20:16
@joanlopez joanlopez self-assigned this Jan 11, 2024
@joanlopez joanlopez requested a review from a team as a code owner January 11, 2024 20:16
@joanlopez joanlopez requested review from mstoykov and oleiade and removed request for a team January 11, 2024 20:16
api/server.go Outdated
@@ -42,6 +43,7 @@ func injectProfilerHandler(mux *http.ServeMux, profilingEnabled bool) {
}

mux.Handle("/debug/pprof/", handler)
mux.Handle("/metrics", promhttp.Handler())
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have reused the --profiling-enabled flag, but I'm also happy with any other, separate, suggestion.

My rationale is that the potential user or use case for both probably overlaps most of the cases, like trying to understand how k6 behaves in terms of resource utilization, etc.

Copy link
Member

Choose a reason for hiding this comment

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

I actually like that the feature lives behind --profiling-enabled 👍🏻 🙇🏻

@codecov-commenter
Copy link

codecov-commenter commented Jan 11, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (32c5f12) 73.07% compared to head (6404a61) 73.03%.

❗ Current head 6404a61 differs from pull request most recent head d4d96f4. Consider uploading reports for the commit d4d96f4 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3535      +/-   ##
==========================================
- Coverage   73.07%   73.03%   -0.05%     
==========================================
  Files         281      279       -2     
  Lines       21102    21098       -4     
==========================================
- Hits        15421    15408      -13     
- Misses       4705     4710       +5     
- Partials      976      980       +4     
Flag Coverage Δ
macos ?
ubuntu 73.03% <100.00%> (+0.02%) ⬆️
windows ?

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

oleiade
oleiade previously approved these changes Jan 15, 2024
Copy link
Member

@oleiade oleiade left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks a lot for acting on this, much appreciated 🙇🏻 🚀

@mstoykov
Copy link
Contributor

mstoykov commented Feb 9, 2024

as part of github.com/prometheus/client_golang is already an -indirect- dependency of the project

This is currently through due to us having an experimental prometheus remote-write output. But it is not certain it will stay or will we move fully to oTEL as such I would argue at some point it won't be free at all.

I also really do prefer to just use stdlib options where possible and golang does have expvar and (after some unsuccesful tries)

       mux.Handle("/debug/vars/", expvar.Handler())

Seems to does the job.

@joanlopez
Copy link
Contributor Author

joanlopez commented Feb 12, 2024

TBH, I'm not sure about the usability of expvar compared to promhttp, nor what will be "the new standard" in the near/mid future, but I agree that, considering this has been there for a year without much claim, it's better to start simple and avoid adding new dependencies, if we have an alternative way to expose such data in case we need them for profiling/debugging.

So, I replaced promhttp with expvar

@joanlopez joanlopez requested a review from oleiade February 12, 2024 08:22
@joanlopez joanlopez merged commit 4de08c3 into grafana:master Feb 12, 2024
24 checks passed
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.

Expose runtime Go metrics
4 participants