-
Notifications
You must be signed in to change notification settings - Fork 25.5k
Deny usage of metric attributes known to cause issues. #136478
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?
Conversation
Due to their mapping or cardinality, certain metric attributes are likely to cause issues. Such attribute names are denied by a new assertion Relates to ES-13074
Pinging @elastic/es-core-infra (Team:Core/Infra) |
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
); | ||
|
||
// forbidden attributes known to cause issues due to mapping conflicts or high cardinality | ||
static final Predicate<String> FORBIDDEN_ATTRIBUTE_NAMES = Regex.simpleMatcher( |
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.
TBC this is just assuming the naming that a developer would use, but doesn't guarantee the same metric isn't used with a different name, right?
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.
there's two issues:
- some fields get mapped to timestamps (see ES-13074), we have to prevent usage of these to prevent such metrics from being silently dropped at ingestion. such errors don't surface anywhere unfortunately.
- high cardinality metrics, adding index here just as precaution for this not to happen again. i also tried to forbid *_id as it indicates some higher cardinality label but there's still two usages of it we have to fix first
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.
Wondering if we should change the interfaces to require metric names to be models as enums to prevent uncontrolled cardinality. Though, that's a massive change :(
Due to their mapping or cardinality, certain metric attributes are likely to cause issues.
Such attribute names are denied by a new assertion
Relates to ES-13074