[exporter/elasticsearch] Data stream routing based on data_stream.* attributes
#33794
Conversation
carsonip
force-pushed
the
route-on-data-stream-attributes-with-grouping
branch
from
5d01e7e
to
cf382bc
Compare
| encodeSpan(pcommon.Resource, ptrace.Span, pcommon.InstrumentationScope) ([]byte, error) | ||
| upsertMetricDataPoint(map[uint32]objmodel.Document, pcommon.Resource, pcommon.InstrumentationScope, pmetric.Metric, pmetric.NumberDataPoint) error |
There was a problem hiding this comment.
Yes, this is ugly. But the "grouping" idea doesn't fit into the separation between exporter and encoding model. Happy to take any suggestions. If it isn't blocking, we can defer it to a future refactoring PR.
There was a problem hiding this comment.
Probably the cleanest separation would be for the encoding model to decide whether it creates one or more documents. Doing this in a followup sounds reasonable.
| encodeSpan(pcommon.Resource, ptrace.Span, pcommon.InstrumentationScope) ([]byte, error) | ||
| upsertMetricDataPoint(map[uint32]objmodel.Document, pcommon.Resource, pcommon.InstrumentationScope, pmetric.Metric, pmetric.NumberDataPoint) error |
There was a problem hiding this comment.
Probably the cleanest separation would be for the encoding model to decide whether it creates one or more documents. Doing this in a followup sounds reasonable.
| return comp | ||
| } | ||
|
|
||
| func assertItemsEqual(t *testing.T, expected, actual []itemRequest, assertOrder bool) { |
There was a problem hiding this comment.
Probably a followup, but seems like we could get by with assert.ElementsMatch if we were to convert the raw JSON to strings, and compare those. I expect the error messages would be more useful then too.
| // Fetch value according to precedence and default. | ||
| value := getFromAttributesNew(attributeName, defaultValue, recordAttributes, scopeAttributes, resourceAttributes) | ||
|
|
||
| // Always set the value on the record, as record attributes have the highest precedence. |
There was a problem hiding this comment.
| // Always set the value on the record, as record attributes have the highest precedence. | |
| // Set the attribute in case the default value is used. | |
| // | |
| // Always set the value on the record, as record attributes have the highest precedence. |
I think? Should we only do this if value == defaultValue?
There was a problem hiding this comment.
Scope attributes are not read in the encodeModel, meaning that only Resource and DataPoint attributes are added to the Document. This is aligned with the behavior of apm-data, where scope attributes are only read for data_stream.* and anything else is ignored, see code. Let me know if you find this behavior problematic.
Should we only do this if value == defaultValue
Therefore, the problem of doing this is that if data_stream.* is present in scope attributes, they will be missing from the resulting doc.
There was a problem hiding this comment.
I see, thanks. Seems odd, but not something we need to change right now.
carsonip
changed the title
data_stream.* attributes
… attributes (open-telemetry#33794) **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.--> Taking over from open-telemetry#33755 - Add data stream routing based on `data_stream.*` attributes - Refine metrics grouping to work with DS routing **Link to tracking Issue:** Closes open-telemetry#33755 Fixes open-telemetry#33756 **Testing:** <Describe what testing was performed and which tests were added.> See unit tests **Documentation:** <Describe the documentation added.> Updated readme --------- Co-authored-by: Andrzej Stencel <[email protected]> Co-authored-by: Andrew Wilkins <[email protected]>
… attributes (open-telemetry#33794) **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.--> Taking over from open-telemetry#33755 - Add data stream routing based on `data_stream.*` attributes - Refine metrics grouping to work with DS routing **Link to tracking Issue:** Closes open-telemetry#33755 Fixes open-telemetry#33756 **Testing:** <Describe what testing was performed and which tests were added.> See unit tests **Documentation:** <Describe the documentation added.> Updated readme --------- Co-authored-by: Andrzej Stencel <[email protected]> Co-authored-by: Andrew Wilkins <[email protected]>
… attributes (open-telemetry#33794) **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.--> Taking over from open-telemetry#33755 - Add data stream routing based on `data_stream.*` attributes - Refine metrics grouping to work with DS routing **Link to tracking Issue:** Closes open-telemetry#33755 Fixes open-telemetry#33756 **Testing:** <Describe what testing was performed and which tests were added.> See unit tests **Documentation:** <Describe the documentation added.> Updated readme --------- Co-authored-by: Andrzej Stencel <[email protected]> Co-authored-by: Andrew Wilkins <[email protected]>
Description:
Taking over from #33755
data_stream.*attributesLink to tracking Issue:
Closes #33755
Fixes #33756
Testing:
See unit tests
Documentation:
Updated readme