-
Notifications
You must be signed in to change notification settings - Fork 25.6k
WIP: Fix downsampling passthrough fields #134996
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
WIP: Fix downsampling passthrough fields #134996
Conversation
| } | ||
| } | ||
| }, | ||
| "metrics.cpu_usage": { |
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.
Let's add a passthrough for metrics, without time_series_dimension set.
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.
Isn't this covered by testDownsamplingPassthroughMetrics?
| addDynamicTemplateForStrings(downsampledMapping); | ||
| for (Map.Entry<String, Object> entry : sourceIndexMappings.entrySet()) { | ||
| if (entry.getKey().equals(MAPPING_DYNAMIC_TEMPLATES)) { | ||
| List<?> sourceDynamicTemplates = (List<?>) sourceIndexMappings.get(MAPPING_DYNAMIC_TEMPLATES); |
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 wonder, do we even need this for the downsample index? @martijnvg too.
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.
This is an interesting approach. I wonder why we didn't something like this in the first place.
…pers under certain conditions. (#135431) This PR is an alternative of #134996: This fixes a bug that would be triggered when we would downsample an index that had a metric field under a passthrough field. For example: ``` "metrics": { "type": "passthrough", "priority": 10, "properties": { "cpu_usage": { "type": "double", "time_series_metric": "gauge" } } } ``` In this PR, we extend the merging possibilities of a passthrough field to allow merging with object mappers that have subobjects false and they are not a root object. They also fix merging an object mapper with a passthrough field and ensure that the result will either be a passthrough field or an error if the objects are incompatible.
…pers under certain conditions. (elastic#135431) This PR is an alternative of elastic#134996: This fixes a bug that would be triggered when we would downsample an index that had a metric field under a passthrough field. For example: ``` "metrics": { "type": "passthrough", "priority": 10, "properties": { "cpu_usage": { "type": "double", "time_series_metric": "gauge" } } } ``` In this PR, we extend the merging possibilities of a passthrough field to allow merging with object mappers that have subobjects false and they are not a root object. They also fix merging an object mapper with a passthrough field and ensure that the result will either be a passthrough field or an error if the objects are incompatible.
|
Alternative fix in #135431 |
This fixes a bug that would be triggered when we would downsample an index that had a metric field under a passthrough field. For example:
In this PR that is still WIP because we are checking if we are missing something, we try to simplify the map update by changing the fields that need adjustment directly on the source mapping. This allows us to eliminate the merge step and create the final mapping with one pass.