Rework metrics to use metrics crate #361
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ludfjig
force-pushed
the
simplify_metrics
branch
from
937173f to
484678c
Compare
ludfjig
changed the title
ludfjig
force-pushed
the
simplify_metrics
branch
from
484678c to
6e131b8
Compare
ludfjig
added
kind/enhancement
ludfjig
force-pushed
the
simplify_metrics
branch
from
6e131b8 to
2d829d6
Compare
ludfjig
added
the
kind/enhancement
ludfjig
force-pushed
the
simplify_metrics
branch
7 times, most recently
from
f523fdb to
411b9f6
Compare
ludfjig
changed the title
ludfjig
force-pushed
the
simplify_metrics
branch
from
411b9f6 to
1518045
Compare
simongdavies
previously requested changes
Apr 16, 2025
There was a problem hiding this comment.
A couple of general questions:
Did we get rid of Gauges? There was a use of them previously and if I am not mistaken somewhere we have an issue that asks for a running total of memory in use, in addition the intent was that these could be used by things built on top of hyperlight such as hyperlight-wasm (we should maintain consistency in the way metrics are collected and emitted between hyperlight and crates built on top)
I may have commented on this in the code review but I dont see how a crate such as hyperlight-wasm would be able to use the metrics structs and macros?
ludfjig
force-pushed
the
simplify_metrics
branch
2 times, most recently
from
0b50f7a to
704a954
Compare
ludfjig
force-pushed
the
simplify_metrics
branch
2 times, most recently
from
b5f1cf5 to
f4e48cc
Compare
Signed-off-by: Ludvig Liljenberg <[email protected]>
Signed-off-by: Ludvig Liljenberg <[email protected]>
…irectly Signed-off-by: Ludvig Liljenberg <[email protected]>
ludfjig
force-pushed
the
simplify_metrics
branch
from
f4e48cc to
1cc9f83
Compare
|
I totally changed the direction of this PR. We now simply use the macros directly where we want to emit metrics. I dont think we need any further abstractions @simongdavies |
Signed-off-by: Ludvig Liljenberg <[email protected]>
ludfjig
force-pushed
the
simplify_metrics
branch
from
758da63 to
047baf8
Compare
danbugs
approved these changes
Apr 17, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This simplifies metrics a lot by using the
metricscrate. Themetricscrate functions similar to thelogcrate in that libraries just callcounter!,gauge!orhistogram!to record metrics, and the user of the library can decide where to send the metrics by installing an appropriate recorder/exporter. If no exporter is installed, the overhead to call the macros are minimal.NEW: got rid of the "nice" apis previously suggested, and now uses the metrics' crates macros directly. Hyperlight-wasm can easily do the same.