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

Define metrics using Prometheus.unsafeRegister instead of having the metrics-core wrapper #4085

Merged
merged 5 commits into from
Jun 17, 2024

Conversation

akshaymankar
Copy link
Member

@akshaymankar akshaymankar commented Jun 6, 2024

The metrics-core package provided a wrapper around metrics which allowed for metrics reuse using 3 IORef (HashMap Text <metric-type>) values. This makes the code much safer than the default way of using the Promehtheus library but also doesn't allow for having Labels and makes using metrics clunkier. It is possible to make these lookups type level and get safe Labels, but it just feels wrong to put so much work into this given metrics are not super important part of the business logic and they are never reused from different places. It is easier to the just use unsafeRegister once on a top level binding.

This PR is a result of me getting carried away and seeing how long it would take me to rework everything, it took ~4 hours. It is completely fine to throw it away and my feeling won't be hurt if we decide this is not a good idea (please, really don't worry about it).

More work on this PR:

  • Replace all dots in the metric names with underscores, they aren't supported by prometheus and get replaced at runtime I tried this, the dots are helpful for people who read the code, so let's leave them.
  • Verify that all the top level binds have a NOINLINE pragma

https://wearezeta.atlassian.net/browse/WPB-9680

Checklist

  • Add a new entry in an appropriate subdirectory of changelog.d
  • Read and follow the PR guidelines

Instead of relying on `Metrics`, use top-level metric registered using
`unsafeRegister`.
@zebot zebot added the ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist label Jun 6, 2024
@akshaymankar akshaymankar marked this pull request as ready for review June 12, 2024 08:58
Copy link
Contributor

@fisx fisx left a comment

Choose a reason for hiding this comment

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

if we forget to inline a metric, it will always only remember the last update, and the internal STVar in prometheus will grow lineary with the number of updates. but i'd expect this to get exposed during testing & QA.

otherwise good! much nicer code :) looking forward to rewriting it again and moving it to wire-subsystems.

@akshaymankar akshaymankar merged commit c2bb1a2 into develop Jun 17, 2024
9 checks passed
@akshaymankar akshaymankar deleted the unsafe-metrics branch June 17, 2024 09:19
@akshaymankar akshaymankar changed the title Proposal: Define metrics using Prometheus.unsafeRegister instead of having the metrics-core wrapper Define metrics using Prometheus.unsafeRegister instead of having the metrics-core wrapper Jun 17, 2024
@echoes-hq echoes-hq bot added the echoes: technical-roadmap/throughput More specific category, to highlight task aiming at improving the development velocity and effici... label Jun 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
echoes: technical-roadmap/throughput More specific category, to highlight task aiming at improving the development velocity and effici... ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants