refactor: type safe metric labels#6201
Merged
wemeetagain merged 13 commits intounstablefrom Jan 2, 2024
Merged
Conversation
nflaig
commented
Dec 18, 2023
| } | ||
| } | ||
|
|
||
| export class GaugeChild<T extends string> implements IGauge { |
Member
Author
There was a problem hiding this comment.
Dropped unused child metrics to get closer to default prom-client interface
| * - Add multiple collect functions after instantiation | ||
| * - Create child histograms with fixed labels | ||
| */ | ||
| export class HistogramExtra<T extends string> extends Histogram<T> implements IHistogram { |
Member
Author
There was a problem hiding this comment.
All this did was add child metric but this should not be required (same as GaugeExtra child)
| }), | ||
| }, | ||
|
|
||
| gossipValidationAccept: register.gauge<"topic">({ |
Member
Author
There was a problem hiding this comment.
Dropped duplicate metrics, see previous related fix #6120
| }, | ||
| config: workerData.config, | ||
| metricsRegistry, | ||
| metricsRegistry: metricsRegistry as IDiscv5CreateOptions["metricsRegistry"], |
Member
Author
There was a problem hiding this comment.
discv5 metrics require collect function to be defined, see discv5/src/metrics.ts#L17. Can clean this up once we reuse same metric types across all packages
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## unstable #6201 +/- ##
============================================
- Coverage 80.95% 80.89% -0.06%
============================================
Files 185 185
Lines 17935 17880 -55
Branches 1078 1078
============================================
- Hits 14519 14464 -55
Misses 3389 3389
Partials 27 27 |
Contributor
Performance Report✔️ no performance regression detected 🚀🚀 Significant benchmark improvement detected
Full benchmark results
|
nazarhussain
requested changes
Dec 19, 2023
| dec: NoLabels extends Labels ? (value?: number) => void : (labels: Labels, value?: number) => void; | ||
| set: NoLabels extends Labels ? (value: number) => void : (labels: Labels, value: number) => void; | ||
|
|
||
| collect?(): void; |
Contributor
There was a problem hiding this comment.
Should not we add deprecated warning here as we did for GuageExtra?
Member
Author
There was a problem hiding this comment.
Overriding collect is the way to go if you use default prom-client Gauge. If we use the custom GaugeExtra on the other hand we should use addCollect instead. It would be good if we move away from GaugeExtra in the long-term and only use prom-client default metric classes.
I completely removed the collect method from GaugeExtra now as there is no reason to use it there.
see commit 2e7d70b
| startTimer(): NoLabels extends Labels ? () => number : (labels: Labels) => number; | ||
| startTimer(labels: NoLabels extends Labels ? undefined : Labels): (labels?: Labels) => number; | ||
|
|
||
| inc: NoLabels extends Labels ? (value?: number) => void : (labels: Labels, value?: number) => void; |
Contributor
There was a problem hiding this comment.
Didn't get that one. If we have the Labels type with multiple keys, do we have to increment every label the same time, else this type will raise error.
Member
Author
There was a problem hiding this comment.
do we have to increment every label the same time, else this type will raise error.
I am not sure what you mean by "increment every label at the same time". What this does is that if you metric has a label defined, then when setting a value for that metric it must specify a label.
Let's look at an example
requestErrors: register.gauge<{routeId: string}>({
name: "vc_rest_api_client_request_errors_total",
help: "Total count of errors on REST API client requests by routeId",
labelNames: ["routeId"],
}),This is no longer allowed
// An argument for 'labels' was not provided.
this.metrics?.requestErrors.inc();You are forced to do
this.metrics?.requestErrors.inc({routeId});I don't see any reason why you would want to set this metric without a label, it would mean there is a unlabeled value which could be combined with further unlabeled values of the same metric but this defeats the purpose of adding a label in the first place, to label and separate values of the same metric.
I only found a few cases were we forgot to set the label but fixed those in this PR as well.
Note that there is also still the option to make the label optional when defining the type for it, I don't think there is any reason to do this but it's an option.
| export type GaugeConfig<Labels extends LabelsGeneric> = { | ||
| name: string; | ||
| help: string; | ||
| } & (NoLabels extends Labels ? {labelNames?: never} : {labelNames: [LabelKeys<Labels>, ...LabelKeys<Labels>[]]}); |
Contributor
There was a problem hiding this comment.
This type is same as:
Suggested change
| } & (NoLabels extends Labels ? {labelNames?: never} : {labelNames: [LabelKeys<Labels>, ...LabelKeys<Labels>[]]}); | |
| } & (NoLabels extends Labels ? {labelNames?: never} : {labelNames: LabelKeys<Labels>[]}); |
Member
Author
There was a problem hiding this comment.
The behavior is different
labelNames: LabelKeys<Labels>[] allows empty arrays
blockProductionTime: register.histogram<{source: ProducedBlockSource}>({
// ...
labelNames: [], // No complaints
}),whereas labelNames: [LabelKeys<Labels>, ...LabelKeys<Labels>[]] requires you to actually add the label name
blockProductionTime: register.histogram<{source: ProducedBlockSource}>({
// ...
labelNames: [], // Type '[]' is not assignable to type '["source", ..."source"[]]'.
}),The ...LabelKeys<Labels>[] part is required for cases were we have multiple labels
packages/utils/src/metrics.ts
Outdated
Comment on lines
+68
to
+70
| gauge<Labels extends LabelsGeneric>(config: GaugeConfig<Labels>): Gauge<Labels>; | ||
| histogram<Labels extends LabelsGeneric>(config: HistogramConfig<Labels>): Histogram<Labels>; | ||
| counter<Labels extends LabelsGeneric>(config: CounterConfig<Labels>): Counter<Labels>; |
Contributor
There was a problem hiding this comment.
Should we not set default value for generics here, else we have to specify type everywhere we use the register.
Member
Author
There was a problem hiding this comment.
You don't have to specify the type explicitly, it is inferred from the configuration
e.g. see histogram usage without labels
But I added them now to address the previous issue regarding usage of {} type #6201 (comment)
see commit 2a14a4b
nazarhussain
approved these changes
Dec 29, 2023
wemeetagain
approved these changes
Jan 2, 2024
Member
wemeetagain
left a comment
wemeetagain
left a comment
There was a problem hiding this comment.
This is nice, looks like it caught a few cases of us missing labels.
I think eventually we should move the metrics utils out of the monorepo so it can be used elsewhere without being pegged to the lodestar release cadence.
Member
Author
That's a good idea and better than having a lodestar/metrics package but I am not sure if a separate package is worth it right now as it would basically just have type definitions and the metrics factory. If we would like to support other metrics like OTLP then it could make sense to have a package that also includes a shim for it. Next step would be to align metric types in other packages and then it should be quite easy to update those once we have a separate metrics package. |
This was referenced Jan 3, 2024
Merged
Member
|
🎉 This PR is included in v1.14.0 🎉 |
ensi321
pushed a commit
to ensi321/lodestar
that referenced
this pull request
Jan 22, 2024
* refactor: type safe metric labels * Update metrics after merging unstable * Remove collect method from GaugeExtra * Remove startTimer method from Gauge interface * Default to NoLabels to fix type issues * Allow to partially set labels in startTimer * Fix type compatibility with prom-client Histogram * Sort metric types * chore: update prom-client to v15.1.0 (ChainSafe#6230) * Add metric type tests
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Motivation
Based on changes done in #6145 and #6143 I noticed a few limitations with in our current metrics and how we maintain labels
labelNamescan be omittedAnother issue is that we duplicate metric types a lot, not just in this codebase but also in libraries like discv5 or libp2p-gossipsub. The interface is quite opinionated and requires custom implementations (like
avgMinMaxoraddCollect) at least in libraries we should not use those and keep it minimal. While this PR does not address the interface issue, it splits them into 3 separate types and cleans up some unused code introduced in #2312 which should make it easier to completely drop custom features likeaddCollectas well and just overridecollectdirectly as we do in discv5 service.ts#L197-L200. If we want to further push this could be addressed in a follow up, first need to ensure there are no metrics that require multiple collectors, however there is a risk of overridingcollecttwice which is a disadvantage compared to currentaddCollectbehavior.This is an initial push towards making our metrics more consumer friendly as noted in ChainSafe/js-libp2p-gossipsub#461.
Description
@lodestar/utils(similar to logger types)Most of the changes are related to stricter label types. Type definitions require a more detailed review 👇
lodestar/packages/utils/src/metrics.ts
Lines 9 to 15 in 3b97f36
Further considerations
We should find a way to avoid duplicating metric types in other libraries like discv5 or libp2p-gossipsub. While these could use
@lodestar/utils, we might also consider having a separate / minimal package for just metrics (e.g.@lodestar/metrics) which includes all the types and some custom code likeRegistryMetricCreatorwhich would at least give other consumers an easy way to create the registry if they wanna use metrics our predefined metrics.We could then also look into eventually support more interfaces like OTLP metrics, ChainSafe/js-libp2p-gossipsub#461 (comment). I haven't looked into the difference to make this work but might be easy to just implement a shim for it.