-
Notifications
You must be signed in to change notification settings - Fork 25.6k
OTLP: store units in mappings #134709
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
OTLP: store units in mappings #134709
Conversation
This is enabled by adding a new `dynamic_templates_params` option to the bulk request header. It can be used in combination with the `dynamic_templates` option to provide parameters that can be used within a dynamic template.
The dynamic template can control which parameters it accepts and can provide a default value in case clients don't provide the parameters in the bulk request. If no default value is provided, and the bulk request also doesn't specify the parameter, the value is kept as it is defined in the template (including braces).
For example, the dynamic template can define a `unit` placeholder with an empty string as the default value.
```
{
"dynamic_templates": [
{
"gauge_double": {
"mapping": {
"type": "double",
"time_series_metric": "gauge",
"meta": {
"unit": "{unit:}"
}
}
}
}
]
}
```
Clients can then set provide a value for the unit in the bulk request header like so:
```
{"create": {"dynamic_templates":{"metrics.foo.bar":"gauge_double"},"dynamic_templates_params":{"metrics.foo.bar":{"unit": "By"}}}}
```
|
Pinging @elastic/es-storage-engine (Team:StorageEngine) |
|
Hi @felixbarny, I've created a changelog YAML for you. |
x-pack/plugin/otel-data/src/main/resources/component-templates/[email protected]
Outdated
Show resolved
Hide resolved
|
We could use a similar mechanism to store the metric description.
For units, this shouldn't be a concern. For descriptions, we could just truncate the description if it is larger than 50. But I heard there's desire to drop or increase the limit for |
Also adds support for double braces for existing {name} and {dynamic_type} placeholders
|
It looks reasonable to me. Do we have any performance numbers, wrt impact to ingest throughput? It's noisy, but may be worth an adhoc run to verify that it's not overly slow. |
|
Not yet, but it's on my list. |
|
Based on the results of an ad-hoc run (environment |
server/src/main/java/org/elasticsearch/index/mapper/DynamicTemplate.java
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/action/bulk/BulkRequestParser.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/index/mapper/DocumentParserContext.java
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/index/mapper/DynamicTemplate.java
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/index/mapper/DynamicTemplate.java
Show resolved
Hide resolved
|
|
||
| import static org.elasticsearch.index.mapper.TimeSeriesParams.TIME_SERIES_DIMENSION_PARAM; | ||
|
|
||
| public class DynamicTemplate implements ToXContentObject { |
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.
@martijnvg do you know who owns this? Do we need another team to review dynamic template changes?
romseygeek
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!
server/src/internalClusterTest/java/org/elasticsearch/action/bulk/IncrementalBulkIT.java
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/index/mapper/DynamicTemplate.java
Show resolved
Hide resolved
server/src/test/java/org/elasticsearch/action/index/IndexRequestTests.java
Outdated
Show resolved
Hide resolved
…stTests.java Co-authored-by: Alan Woodward <[email protected]>
This is enabled by adding a new
dynamic_templates_paramsoption to the bulk request header. It can be used in combination with thedynamic_templatesoption to provide parameters that can be used within a dynamic template.The dynamic template can control which parameters it accepts and can provide a default value in case clients don't provide the parameters in the bulk request. If no default value is provided, and the bulk request also doesn't specify the parameter, the value is kept as it is defined in the template (including braces).
For example, the dynamic template can define a
unitplaceholder.Clients can then set provide a value for the unit in the bulk request header like so:
Placeholders for which no parameters are specified will evaluate to an empty string.
I chose a
{{placeholder}}syntax over{placeholder}to avoid ambiguity as{percent}is actually a literal value that we expect to use which is not meant to be interpreted as a placeholder. This is also consistent with the syntax we use in other places like ingest pipelines (mustache and reroute processor placeholders). However, at the moment, dynamic templates support some parameters with only a single pair of braces:{name}and{dynamic_type}. I've added support for specifying these in double braces as well to maintain consistency (while still supporting the single brace version). Interestingly, we've documented that we already support this syntax: https://www.elastic.co/docs/manage-data/data-store/mapping/dynamic-templatesHowever, it's not actually true (yet).