Mapping mode removal #45250
Mapping mode removal #45250
Conversation
… if this is the case
…e from Config file
…ult to OTel and that we are getting a warning message
| elasticsearch: | ||
| endpoint: https://elasticsearch:9200 | ||
| auth: | ||
| authenticator: headers_setter |
There was a problem hiding this comment.
pending testing whether this works
| case 0: | ||
| return e.defaultMappingMode, nil | ||
| if e.defaultMappingMode != MappingOTel { | ||
| e.set.Logger.Warn("mapping mode can no longer be supplied via config file. Use the `X-Elastic-Mapping-Mode` client metadata key or the `elastic.mapping.mode` scope attribute instead.") |
There was a problem hiding this comment.
I was originally a bit conflicted about the per-request warning on deprecated mapping mode but given the impact it might be reasonable.
| if e.defaultMappingMode != MappingOTel { | ||
| e.set.Logger.Warn("mapping mode can no longer be supplied via config file. Use the `X-Elastic-Mapping-Mode` client metadata key or the `elastic.mapping.mode` scope attribute instead.") | ||
| } | ||
| return MappingOTel, nil |
There was a problem hiding this comment.
[likely no-op] If we do not need per-request warning, it'll be better if we propagate the hardcoded default to up to newExporter instead of passing it into getRequestMappingMode then ignoring it.
|
|
||
| Since the `mapping::mode` config option is deprecated, use one of the following methods to set the mapping mode: | ||
|
|
||
| **Option 1: Using client metadata via headers_setter extension** |
There was a problem hiding this comment.
I've tested this and I'm afraid this doesn't work. The headers_setter sets HTTP and gRPC headers to outgoing requests, and in this case, any HTTP requests from es exporter to ES, by wrapping the HTTP roundtrip. Therefore, the header won't be available to es exporter. For header setter to work, it needs to happen before the pipeline, i.e. in otlp exporter with header setter, then in this pipeline's otlp receiver with include_metadata, so that the header is transformed into context metadata, which will then be available to es exporter.
Unless there are easier ways to inject client metadata, I suggest we either remove this option in the section, or simply describe how it needs to happen before the pipeline.
There was a problem hiding this comment.
Thanks for the suggestion, I agree with your statement and think that we should remove this option and keep the other one instead in order to keep things simple.
|
/rerun |
axw
left a comment
axw
left a comment
There was a problem hiding this comment.
@jlind23 code changes look good, but per https://github.com/open-telemetry/opentelemetry-collector/blob/main/docs/component-stability.md#configuration-changes-1 we should deprecate first and make the breaking change (i.e. ignoring config) in a subsequent release.
5 participants
Description
Marked the manually provided mapping mode as deprecated, config can still be supplied but will be ignored.
We will then default to
otelmapping if no header nor attributes are specified.Link to tracking issue
Fixes #45246
Testing
Tests were updated in order to validate the new behavior, tests that were passing option via config file were updated in order to rely on the header instead.
Documentation
Elasticsearch exporter readme file was updated in order to add migration steps to help users move away from file provided configuration.