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] Workaround TSDB array dimension limitation for metrics OTel mode #35009

Conversation

carsonip
Copy link
Contributor

@carsonip carsonip commented Sep 4, 2024

Description:

Elasticsearch TSDB does not support array dimensions. Workaround it by stringifying attribute array values in OTel mapping mode for metrics.

Refactor to improve test code reuse.

Link to tracking Issue:

Fixes #35004

Testing:

Added exporter test for otel mode logs, metrics and traces

Documentation:

@carsonip carsonip changed the title Workaround TSDB array dimension limitation for metrics OTel mode [exporter/elasticsearch] Workaround TSDB array dimension limitation for metrics OTel mode Sep 4, 2024
document.AddAttributes("attributes", attributeMap)
}

func mapStringifyArrayValues(m pcommon.Map) {
m.Range(func(k string, v pcommon.Value) bool {
if v.Type() == pcommon.ValueTypeSlice {
Copy link
Contributor

Choose a reason for hiding this comment

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

This not only affects array values but also other complex attribute values, like ValueTypeMap. Not quite sure how to deal with ValueTypeBytes tbh. How do we serialize it normally? Is that compatible with a field type that supports dimensions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

according to https://opentelemetry.io/docs/specs/otel/common/

The attribute value is either:

A primitive type: string, boolean, double precision floating point (IEEE 754-1985) or signed 64 bit integer.
An array of primitive type values. The array MUST be homogeneous, i.e., it MUST NOT contain values of different types.

I don't think ValueTypeMap and ValueTypeBytes fall into these valid value types.

Copy link
Contributor

Choose a reason for hiding this comment

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

How are we dealing with cases where we get invalid attribute values? Are we ignoring them?

Copy link
Contributor Author

@carsonip carsonip Sep 5, 2024

Choose a reason for hiding this comment

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

We had a similar issue in apm-server where otlp events with non-compliant attributes cause panics: elastic/apm-server#13703

In es exporter, by looking at the code, ValueTypeMap will be successfully encoded as objects, ValueTypeBytes will be ignored, non-homogeneous arrays will be successfully encoded. Ideally they should be dropped explicitly with a warning, but as these are non-compliant, user should expect undefined behavior, which includes successful or unsuccessful indexing.

Copy link
Contributor

Choose a reason for hiding this comment

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

As long as we don't serialize non-compliant values and there's no panic, this sounds fine. Ideally, we would increment the dropped attributes count.

@carsonip
Copy link
Contributor Author

carsonip commented Sep 5, 2024

Ideally #35023 is addressed first, so that the impact of #35023 is not pronounced by this change.

@carsonip carsonip marked this pull request as ready for review September 5, 2024 21:55
@carsonip carsonip requested review from a team, songy23 and felixbarny September 5, 2024 21:55
Copy link
Member

@andrzej-stencel andrzej-stencel left a comment

Choose a reason for hiding this comment

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

🚀

@andrzej-stencel andrzej-stencel merged commit abb7604 into open-telemetry:main Sep 6, 2024
156 checks passed
@github-actions github-actions bot added this to the next release milestone Sep 6, 2024
f7o pushed a commit to f7o/opentelemetry-collector-contrib that referenced this pull request Sep 12, 2024
…or metrics OTel mode (open-telemetry#35009)

**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.-->
Elasticsearch TSDB does not support array dimensions. Workaround it by
stringifying attribute array values in OTel mapping mode for metrics.

Refactor to improve test code reuse.

**Link to tracking Issue:** <Issue number if applicable>

Fixes open-telemetry#35004

**Testing:** <Describe what testing was performed and which tests were
added.>

Added exporter test for otel mode logs, metrics and traces

**Documentation:** <Describe the documentation added.>
andrzej-stencel pushed a commit that referenced this pull request Sep 25, 2024
…etrics OTel mode (#35291)

**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.-->
Remove the workaround in #35009 to stringify array dimensions as the
limitation has been lifted in Elasticsearch in
elastic/elasticsearch#110387

This change is not breaking as array will be coerced as string, meaning
that there will be no indexing error. Additionally, the changes are
acceptable as OTel mapping mode explicitly marked as in development.

**Link to tracking Issue:** <Issue number if applicable>

**Testing:** <Describe what testing was performed and which tests were
added.>

**Documentation:** <Describe the documentation added.>

---------

Co-authored-by: Vishal Raj <[email protected]>
jriguera pushed a commit to springernature/opentelemetry-collector-contrib that referenced this pull request Oct 4, 2024
…or metrics OTel mode (open-telemetry#35009)

**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.-->
Elasticsearch TSDB does not support array dimensions. Workaround it by
stringifying attribute array values in OTel mapping mode for metrics.

Refactor to improve test code reuse.

**Link to tracking Issue:** <Issue number if applicable>

Fixes open-telemetry#35004

**Testing:** <Describe what testing was performed and which tests were
added.>

Added exporter test for otel mode logs, metrics and traces

**Documentation:** <Describe the documentation added.>
jriguera pushed a commit to springernature/opentelemetry-collector-contrib that referenced this pull request Oct 4, 2024
…etrics OTel mode (open-telemetry#35291)

**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.-->
Remove the workaround in open-telemetry#35009 to stringify array dimensions as the
limitation has been lifted in Elasticsearch in
elastic/elasticsearch#110387

This change is not breaking as array will be coerced as string, meaning
that there will be no indexing error. Additionally, the changes are
acceptable as OTel mapping mode explicitly marked as in development.

**Link to tracking Issue:** <Issue number if applicable>

**Testing:** <Describe what testing was performed and which tests were
added.>

**Documentation:** <Describe the documentation added.>

---------

Co-authored-by: Vishal Raj <[email protected]>
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] Workaround TSDB limitation on array dimension fields
4 participants