Skip to content
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

[exporter/elasticsearch] Data stream routing based on data_stream.* attributes #33794

Conversation

carsonip
Copy link
Contributor

@carsonip carsonip commented Jun 27, 2024

Description:

Taking over from #33755

  • Add data stream routing based on data_stream.* attributes
  • Refine metrics grouping to work with DS routing

Link to tracking Issue:
Closes #33755
Fixes #33756

Testing:

See unit tests

Documentation:

Updated readme

@carsonip carsonip force-pushed the route-on-data-stream-attributes-with-grouping branch from 5d01e7e to cf382bc Compare June 27, 2024 21:51
exporter/elasticsearchexporter/README.md Outdated Show resolved Hide resolved
encodeSpan(pcommon.Resource, ptrace.Span, pcommon.InstrumentationScope) ([]byte, error)
upsertMetricDataPoint(map[uint32]objmodel.Document, pcommon.Resource, pcommon.InstrumentationScope, pmetric.Metric, pmetric.NumberDataPoint) error
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

exporter/elasticsearchexporter/model_test.go Outdated Show resolved Hide resolved
exporter/elasticsearchexporter/model.go Show resolved Hide resolved
Copy link
Contributor

@axw axw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good overall. I'd really like to avoid more configuration if we can.

exporter/elasticsearchexporter/README.md Outdated Show resolved Hide resolved
encodeSpan(pcommon.Resource, ptrace.Span, pcommon.InstrumentationScope) ([]byte, error)
upsertMetricDataPoint(map[uint32]objmodel.Document, pcommon.Resource, pcommon.InstrumentationScope, pmetric.Metric, pmetric.NumberDataPoint) error
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

exporter/elasticsearchexporter/model.go Show resolved Hide resolved
return comp
}

func assertItemsEqual(t *testing.T, expected, actual []itemRequest, assertOrder bool) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// 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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, thanks. Seems odd, but not something we need to change right now.

exporter/elasticsearchexporter/data-stream-router.go Outdated Show resolved Hide resolved
@carsonip carsonip marked this pull request as ready for review June 28, 2024 10:15
@carsonip carsonip requested review from a team and MovieStoreGuy June 28, 2024 10:15
Copy link
Contributor

@axw axw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - we should reenable TestEncodeMetric

exporter/elasticsearchexporter/exporter.go Outdated Show resolved Hide resolved
@carsonip carsonip changed the title Data stream routing with metrics grouping [exporter/elasticsearch] Data stream routing based on data_stream.* attributes Jun 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[exporter/elasticsearch] Group metrics in documents based on timestamp and attributes
7 participants