Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Use package local metrics #1578

Merged
merged 2 commits into from
Sep 4, 2024
Merged

Use package local metrics #1578

merged 2 commits into from
Sep 4, 2024

Conversation

SuperQ
Copy link
Contributor

@SuperQ SuperQ commented Aug 19, 2024

Improve performance of metrics by moving them to the package that needs them. This reduces the overhead to a simple atomic increment for basic counters like cache hits/misses. This also uses promauto to avoid the second step of having to register metrics.

Copy link

codecov bot commented Aug 19, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.91%. Comparing base (fe84ab8) to head (23ded2f).
Report is 125 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1578      +/-   ##
==========================================
+ Coverage   93.88%   93.91%   +0.03%     
==========================================
  Files          78       79       +1     
  Lines        6361     5078    -1283     
==========================================
- Hits         5972     4769    -1203     
+ Misses        300      218      -82     
- Partials       89       91       +2     

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

Copy link
Collaborator

@ThinkChaos ThinkChaos left a comment

Choose a reason for hiding this comment

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

LGTM, would be nice to move the other metrics too, but @0xERR0R should chime in, as mentioned in #1560, before you do more work :)

@SuperQ
Copy link
Contributor Author

SuperQ commented Aug 27, 2024

Yes, I did this PR as an example of the re-architecture. I'm happy to contribute more improvements.

@ThinkChaos
Copy link
Collaborator

ThinkChaos commented Aug 27, 2024

Looks like the end to end test for metrics failed though, so it might now fully work at the moment.

EDIT: from the logs, the missing metrics match the changed ones:

  the missing elements were
      <[]string | len:2, cap:2>: [
          "blocky_cache_hits_total 0",
          "blocky_cache_misses_total 0",
      ]

@SuperQ
Copy link
Contributor Author

SuperQ commented Aug 28, 2024

Ahh, yes, this is because metrics/metrics.go creates an isolated metrics registry instead of using the standard default registry.

Also because it's not created by the main or exported it's not easy to pass the registry around.

The simplest solution would be to use the default registry.

@SuperQ SuperQ force-pushed the local_metrics branch 2 times, most recently from 1ce9a01 to 1742dce Compare August 28, 2024 06:52
SuperQ added 2 commits August 28, 2024 09:32
Improve performance of metrics by moving them to the package that needs
them. This reduces the overhead to a simple atomic increment for basic
counters like cache hits/misses. This also uses `promauto` to avoid the
second step of having to register metrics.

Signed-off-by: SuperQ <superq@gmail.com>
Export the custom metrics registry from the metrics package so it can be
used with promauto in other packages.

Signed-off-by: SuperQ <superq@gmail.com>
@SuperQ
Copy link
Contributor Author

SuperQ commented Aug 28, 2024

I went another direction and simply exported the registry from the metrics package.

@0xERR0R 0xERR0R added the 🧰 technical debts Technical debts, refactoring label Sep 4, 2024
@0xERR0R 0xERR0R added this to the v0.25 milestone Sep 4, 2024
Copy link
Owner

@0xERR0R 0xERR0R left a comment

Choose a reason for hiding this comment

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

👍

@0xERR0R 0xERR0R merged commit a2d7daa into 0xERR0R:main Sep 4, 2024
11 checks passed
SuperQ added a commit to SuperQ/blocky that referenced this pull request Sep 13, 2024
Part 2 (0xERR0R#1578)

Improve performance of metrics by moving them to the package that needs
them. This reduces the overhead to a simple atomic increment for basic
counters like cache hits/misses. This also uses `promauto` to avoid the
second step of having to register metrics.

Signed-off-by: SuperQ <superq@gmail.com>
SuperQ added a commit to SuperQ/blocky that referenced this pull request Sep 13, 2024
Part 2 (0xERR0R#1578)

Improve performance of metrics by moving them to the package that needs
them. This reduces the overhead to a simple atomic increment for basic
counters like cache hits/misses. This also uses `promauto` to avoid the
second step of having to register metrics.

Signed-off-by: SuperQ <superq@gmail.com>
SuperQ added a commit to SuperQ/blocky that referenced this pull request Sep 13, 2024
Part 2 (0xERR0R#1578)

Improve performance of metrics by moving them to the package that needs
them. This reduces the overhead to a simple atomic increment for basic
counters like cache hits/misses. This also uses `promauto` to avoid the
second step of having to register metrics.

Signed-off-by: SuperQ <superq@gmail.com>
SuperQ added a commit to SuperQ/blocky that referenced this pull request Oct 29, 2024
Part 2 (0xERR0R#1578)

Improve performance of metrics by moving them to the package that needs
them. This reduces the overhead to a simple atomic increment for basic
counters like cache hits/misses. This also uses `promauto` to avoid the
second step of having to register metrics.

Signed-off-by: SuperQ <superq@gmail.com>
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
🧰 technical debts Technical debts, refactoring
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants