[exporter/elasticsearch] Add OTel mapping mode for metrics #34248
Conversation
carsonip
force-pushed
the
otel-mode-metrics
branch
from
13a3f58 to
5815374
Compare
carsonip
force-pushed
the
otel-mode-metrics
branch
from
e1d0fa6 to
786d0f0
Compare
This reverts commit 98f5d7c.
carsonip
changed the title
| // Move them to the top of the document and remove them from the record | ||
| attributeMap := dp.Attributes() | ||
|
|
||
| forEachDataStreamKey := func(fn func(key string)) { |
There was a problem hiding this comment.
Refactoring done in #34472
| return nil | ||
| } | ||
|
|
||
| func metricDpToDynamicTemplate(metric pmetric.Metric, dp dataPoint) string { |
There was a problem hiding this comment.
This might be a little bit hard to understand for someone not already familiar with all the details. Perhaps add a link to the metrics@mappings template, and explain that we're telling Elasticsearch which template to use for dynamic mapping?
| mapHashExcludeDataStreamAttr(hasher, scopeAttrs) | ||
| mapHashExcludeDataStreamAttr(hasher, dp.Attributes()) |
There was a problem hiding this comment.
We're excluding them because docs are already grouped by resource, and data_stream.* is set in the resource attributes by the document router - right? Maybe worth another comment here?
There was a problem hiding this comment.
I'll add a comment. It is because e.g. if both scope DS dataset and record level DS dataset are present, scope DS dataset will effectively be ignored, and will not affect the resulting document, as the record level one will be used. We'll risk duplicates if we factor it in during hashing.
There was a problem hiding this comment.
On the other hand, documents are already passed into the function is already "sharded" by index name, which is also a reason why it isn't needed in hashing.
There was a problem hiding this comment.
thanks for making me think more about it. ECS mode should use this mapHashExcludeDataStreamAttr as well.
| component: elasticsearchexporter | ||
|
|
||
| # A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`). | ||
| note: Add OTel mapping mode for metrics |
There was a problem hiding this comment.
that's a bit mysterious, can you add to what mapping we map to?
| note: Add OTel mapping mode for metrics | |
| note: Add support for Elastic ECS data model mapping from OpenTelemetry metrics. See <URL> for the mapping. |
There was a problem hiding this comment.
It is not related to ECS data model. See https://github.com/open-telemetry/opentelemetry-collector-contrib/pull/34472/files#r1718710444 for explanation.
There was a problem hiding this comment.
Would be good to update the "Exporting metrics" section of the exporter's README to document which mapping modes are supported for metrics (ECS, OTel) and how they work.
Unrelated to this PR, after #34045 the README incorrectly says that only Sum and Gauge metric types are supported, this should also be updated.
…metry#34248) **Description:** <Describe what has changed.> <!--Ex. Fixing a bug - Describe the bug and how this fixes the issue. Ex. Adding a feature - Explain what this achieves.--> Add OTel mapping mode for metrics. OTel mapping mode is a new mapping mode described in https://github.com/open-telemetry/opentelemetry-collector-contrib/tree/main/exporter/elasticsearchexporter#elasticsearch-document-mapping **Link to tracking Issue:** <Issue number if applicable> **Testing:** <Describe what testing was performed and which tests were added.> Added exporter test **Documentation:** <Describe the documentation added.>
Description:
Add OTel mapping mode for metrics.
OTel mapping mode is a new mapping mode described in https://github.com/open-telemetry/opentelemetry-collector-contrib/tree/main/exporter/elasticsearchexporter#elasticsearch-document-mapping
Link to tracking Issue:
Testing:
Added exporter test
Documentation: