-
Notifications
You must be signed in to change notification settings - Fork 127
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
feat: [plugin/prometheus] allow to skip observation based on context #2317
base: main
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: 27b09e2 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
💻 Website PreviewThe latest changes are available as preview in: https://75427464.envelop.pages.dev |
✅ Benchmark Results
|
@klippx Hey! What do you think of this new configuration API ? Would it cover you needs ? |
fillLabelsFn: ({ operationName }, _rawContext) => ({ | ||
operation_name: operationName | ||
}), | ||
shouldObserve: context => TRACKED_OPERATIONS.includes(context?.params?.operationName) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like it!
But I think the example should be updated:
shouldObserve: context => TRACKED_OPERATIONS.includes(context?.params?.operationName) | |
shouldObserve: (_, context) => TRACKED_OPERATIONS.includes(context?.params?.operationName) |
because I think the signature is the same as fillLabelsFn
.
LabelNames extends string, | ||
Params extends Record<string, any> = FillLabelsFnParams, | ||
>(options: { | ||
registry: Registry; | ||
histogram: Omit<HistogramConfiguration<LabelNames>, 'registers'>; | ||
fillLabelsFn: FillLabelsFn<LabelNames, Params>; | ||
}): HistogramAndLabels<LabelNames, Params> { | ||
phases: Phases; | ||
shouldObserve?: ShouldObservePredicate<Params>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you ensure that if I hover this a helpful jsdoc
will appear to explains what this option is doing, and what the default is?
Such as
/**
* A function that determines whether the metric should be recorded or not.
*
* Defaults to `() => true`.
*/
Might need to be repeated 2 other places below.
I think this would cover my needs for sure, it looks great! |
Description
This PR aims to give more control over exposed metrics.
Today, the configuration is very confusing about what metrics are enabled or not, which can lead to silently broken configuration.
I propose a new way to configure which metrics should be enabled, for which execution phases, with which labels. The goal is also to offer a more dynamic approach by giving a way to skip metric observation based on execution context.
Fixes #2312
Type of change
Please delete options that are not relevant.