-
Notifications
You must be signed in to change notification settings - Fork 684
distributor: retrieve validation config once per request, not per sample #13807
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
base: main
Are you sure you want to change the base?
distributor: retrieve validation config once per request, not per sample #13807
Conversation
Signed-off-by: Oleg Zaytsev <[email protected]>
Signed-off-by: Oleg Zaytsev <[email protected]>
Signed-off-by: Oleg Zaytsev <[email protected]>
Signed-off-by: Oleg Zaytsev <[email protected]>
tacole02
left a comment
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.
Changelog LGTM
Signed-off-by: Oleg Zaytsev <[email protected]>
jesusvazquez
left a comment
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.
LGTM
| } | ||
|
|
||
| // newValidationConfig builds a validationConfig based on the passed overrides. | ||
| // TODO: This could still be more efficient, as each overrides call performs an atomic pointer retrieval and a lookup in a map, |
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.
What are your thoughts on possibly making this more efficient in the future? Are you thinking about a low TTL cache to avoid the map calls? I was thinking about this but it could get tricky with invalidations if one of the fields changes.
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 was thinking about adding an Overrides.Visit(userID string, func(limits *Limits)) that would retrieve the *Limits from runtime config once, and call the callback which would be used to fill our config.
However, some configs have some extra logic in Overrides, and letting this code access fields of *Limits seems like leaking responsibilities.
It would make sense to refactor the entire Overrides so methods would be on the Limits struct, not on the overrides itself, and Overrides would delegate the calls to *Limits, then we use that here.
Anyway, this is good enough, let's revisit in 2032.
What this PR does
Builds a
validationConfigstruct that is populated once per request instead of calling thevalidation.Overridesmethods with user ID multiple times for each one of the samples, series, and metadata.We've seen that we're doing millions of these calls per distributor. @seizethedave did a huge improvement in grafana/dskit#843 that made the underlying
GetConfigcall cheaper, however I wanted to go deeper and avoid the atomic call and the map lookup at all.Which issue(s) this PR fixes or relates to
Fixes N/A
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]. If changelog entry is not needed, please add thechangelog-not-neededlabel to the PR.about-versioning.mdupdated with experimental features.Note
Build and reuse a per-request validation config to avoid repeated overrides lookups, refactoring distributor validation paths and tests accordingly.
validationConfigonce per request vianewValidationConfig(userID, overrides)and reuse across series/metadata validation.validateSeries→validateSamples/validateHistograms/validateLabels/cleanAndValidateMetadata.sampleValidationConfig,labelValidationConfig,metadataValidationConfigusing pre-resolved fields.validateSample,validateSampleHistogram,validateLabels,cleanAndValidateMetadata) to read fields directly from struct.validationConfig.TestNewValidationConfigFieldCompletenessto ensure allvalidationConfigfields are populated.Written by Cursor Bugbot for commit 8e46084. This will update automatically on new commits. Configure here.