[exporter/elasticsearch] Handle edge cases in metrics grouping causing TSDB version_conflict_engine_exception #39309

carsonip · 2025-04-10T19:12:14Z

Description

Fix 2 edge cases where metrics grouping were not done correctly.

In ecs mode, metric grouping now considers data point attributes overwriting resource attributes.
In ecs and otel mode, attributes are now sorted before hashing. Metrics with attributes but in different order are now grouped together.

Implementation:

sort attributes
split up resource, scope and dp hash computation for otel mode to avoid sorting and rehashing

Link to tracking issue

Fixes #38083

Testing

Added unit tests

exporter/elasticsearchexporter/metric_grouping.go

.chloggen/elasticsearchexporter_handle-metric-grouping-edge-cases.yaml

lahsivjar · 2025-04-11T10:40:01Z

exporter/elasticsearchexporter/metric_grouping.go

 // e.g. index is already considered during routing and DS attributes do not need to be considered in hashing
-func mapHashExcludeReservedAttrs(hasher hash.Hash, m pcommon.Map, extra ...string) {
+func mapHashSortedExcludeReservedAttrs(hasher hash.Hash, m pcommon.Map, extra ...string) {


Did we consider using common libraries like:

https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/main/pkg/pdatautil/hash.go
https://github.com/open-telemetry/opentelemetry-collector-contrib/tree/main/internal/exp/metrics/identity

for the hash calculations? I think almost all the functions here could be replaced by existing hash options. For the exclusion of DataStream* attributes we could create a custom hash option.

Probably not able to use identity directly, since some fields are used but are not TSDB dimensions, e.g. schema url.

pdatautil hash, on the other hand, looks good and will get rid of a lot of the hashing code we have. Still need to look into how to achieve exclusions. I think we'll be better off creating a map and passing it in, instead of writing an Option which means reimplementing all the funcs in pdatautil hash because they are private.

I think we'll be better off creating a map and passing it in

which will involve a fair bit of copying

Probably not able to use identity directly, since some fields are used but are not TSDB dimensions, e.g. schema url.

I thought so, the identity is a bit rigid.

Still need to look into how to achieve exclusions

We can implement a new HashOption (~~in elasticsearch-exporter for now~~) - something like WithMapExclusions(m pcommon.Map, excludedKeys ...string) HashOption

instead of writing an Option which means reimplementing all the funcs in pdatautil hash because they are private

Ah, right!!! everything is private. Maybe we can add a TODO for exporter and create a PR in pdatautil to have this hash option?

Added a TODO, shouldn't be a blocker for this PR.

lahsivjar · 2025-04-11T10:44:59Z

exporter/elasticsearchexporter/metric_grouping.go

+	return 0
+}
+
+func (h ecsDataPointHasher) hashScope(_ pcommon.InstrumentationScope) uint32 {
+	return 0
+}
+
+func (h ecsDataPointHasher) hashDataPoint(_ datapoints.DataPoint) uint32 {
+	return 0


nit: This feels a bit misleading for the API. As an API user, calling hashScope, I would expect to get the hash of the instrumentation scope - using the hash or not should be controlling which hash to keep.

this is my shot at the following requirements

ecs hashes differently from otel as data point attributes and resource attributes are propagated to the root level

hashing of resource should be done per-resource, scope done per-scope, to avoid rehashing and sorting resource attributes and scope attributes per-data point.

(not a strict requirement) avoid leaking ecs vs otel difference to exporter level

I guess the key to the discussion here is where to put the line between exporter logic and metrics grouping logic. Welcome any suggestions

I think the root of the issue is that the mapping mode is leaking in the hashing logic. What do you think about making the dataPointGroupKey aware of the mapping mode and calculate the key based on that. So, for example: for ecs mode, it will populate the combined hash key only, whereas for otel mode it will populate resource and scope keys? This will also mean that we will only need DataPointHasher.

Additionally, check https://github.com/open-telemetry/opentelemetry-collector-contrib/pull/39309/files#r2039284735 - maybe we can remove the *DataPointHasher completely but this is up to further discussion.

What do you think about making the dataPointGroupKey aware of the mapping mode and calculate the key based on that. So, for example: for ecs mode, it will populate the combined hash key only, whereas for otel mode it will populate resource and scope keys? This will also mean that we will only need DataPointHasher.

Let's say there exist a function call getGroupKey(mode, resource, scope, dp) dataPointGroupKey, doesn't it mean that it needs to be called per data point? Won't we lose the optimization to avoid sorting and rehashing resource and scope attributes?

Won't we lose the optimization to avoid sorting and rehashing resource and scope attributes?

We can just have an interface to update the dataPointGroupKey, something like UpdateWithScope, UpdateWithDatapoint...

This is great advice. I've moved hash key to the grouping file to avoid leaking the implementation to exporter. cce4471

exporter/elasticsearchexporter/metric_grouping.go

lahsivjar

Mostly LGTM, just some minor comments.

lahsivjar · 2025-04-11T16:38:42Z

exporter/elasticsearchexporter/internal/metricgroup/hash.go

+// TODO(carsonip): use opentelemetry-collector-contrib/pkg/pdatautil/hash.go when it can optionally exclude attributes
+// We could have used it now but it'll involve creating a new Map and copying things over.


Can we create an issue/pr to track this?

lahsivjar · 2025-04-11T19:30:15Z

exporter/elasticsearchexporter/internal/metricgroup/hasher.go

+type HashKey struct {
+	resourceHash uint32
+	scopeHash    uint32
+	dpHash       uint32
+}


My initial idea was to not have {ecs, otel}DataPointHasher at all and build the hash update logic in the HashKey but I think we can discuss that in the follow-up PR when the pdatautil has the method for hashing map with exclude keys.

lahsivjar · 2025-04-11T19:33:20Z

exporter/elasticsearchexporter/internal/metricgroup/hasher.go

+func (h *ECSDataPointHasher) HashKey() HashKey {
+	merged := pcommon.NewMap()
+	merged.EnsureCapacity(h.resource.Attributes().Len() + h.dp.Attributes().Len())
+	h.resource.Attributes().CopyTo(merged)
+	// scope attributes are ignored in ECS mode
+	for k, v := range h.dp.Attributes().All() {
+		v.CopyTo(merged.PutEmpty(k))
+	}
+
+	hasher := fnv.New32a()
+
+	timestampBuf := make([]byte, 8)
+	binary.LittleEndian.PutUint64(timestampBuf, uint64(h.dp.Timestamp()))
+	hasher.Write(timestampBuf)
+
+	mapHashSortedExcludeReservedAttrs(hasher, merged)
+
+	return HashKey{
+		dpHash: hasher.Sum32(),
+	}


nit: Can we unify the behaviour of HashKey for both the datapoint hashers? Currently, the ECS one is doing hash calculation in the HashKey method whereas the OTel one does it on Update.

carsonip added 11 commits April 10, 2025 16:55

Rename scopeMetric

6484c00

Add group key struct

3ed61c6

Split hashes

f6bd684

Add ecs grouping overwrite test

dc35f1e

Fix ECS attribute overwriting

7589dbc

Add unordered attributes test

4559e9f

Use .All instead of .Range

74601f2

Sort before hashing

69dce9e

Add sanity check

6ceaf0e

Refactor tests

cd83a64

Add changelog

f5a8c8a

carsonip requested a review from a team as a code owner April 10, 2025 19:12

carsonip requested a review from mx-psi April 10, 2025 19:12

github-actions bot assigned MovieStoreGuy Apr 10, 2025

github-actions bot added the exporter/elasticsearch label Apr 10, 2025

github-actions bot requested review from JaredTan95 and lahsivjar April 10, 2025 19:12

carsonip added 2 commits April 10, 2025 20:24

Fix variable shadowing

7f055a8

Ignore error from FromRaw

aa815af

carsonip commented Apr 10, 2025

View reviewed changes

exporter/elasticsearchexporter/metric_grouping.go Outdated Show resolved Hide resolved

carsonip added 3 commits April 10, 2025 20:28

Correct comment

357b5e8

Remove FromRaw AsRaw

9f350e9

Fix test

810dbc6

carsonip changed the title ~~[exporter/elasticsearch] Handle edge cases in metrics grouping to avoid TSDB version_conflict_engine_exception~~ [exporter/elasticsearch] Handle edge cases in metrics grouping causing TSDB version_conflict_engine_exception Apr 11, 2025

carsonip added 2 commits April 11, 2025 10:47

Changelog

bbd4794

Use assert.Len

8fbec70

lahsivjar reviewed Apr 11, 2025

View reviewed changes

carsonip added 3 commits April 11, 2025 12:58

Add TODO

67003e8

Avoid leaking hash implementation to exporter

cce4471

Refactor to internal package

6c9cbc4

carsonip force-pushed the sort-attributes-hash branch from 4b1a72c to 6c9cbc4 Compare April 11, 2025 14:27

carsonip added 7 commits April 11, 2025 16:21

Add TODO

8a7d8be

Fix TODO

31ad7f9

Update comment

4dca251

Update changelog

7ffcaa4

Merge branch 'main' into sort-attributes-hash

c0cbbb1

Add comment

898b228

Make linter happy

6edb721

carsonip requested a review from lahsivjar April 11, 2025 16:12

lahsivjar approved these changes Apr 11, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[exporter/elasticsearch] Handle edge cases in metrics grouping causing TSDB version_conflict_engine_exception #39309

[exporter/elasticsearch] Handle edge cases in metrics grouping causing TSDB version_conflict_engine_exception #39309

carsonip commented Apr 10, 2025 •

edited

Loading

lahsivjar Apr 11, 2025

carsonip Apr 11, 2025

carsonip Apr 11, 2025

lahsivjar Apr 11, 2025 •

edited

Loading

lahsivjar Apr 11, 2025 •

edited

Loading

carsonip Apr 11, 2025

lahsivjar Apr 11, 2025

carsonip Apr 11, 2025

carsonip Apr 11, 2025

lahsivjar Apr 11, 2025

lahsivjar Apr 11, 2025

carsonip Apr 11, 2025 •

edited

Loading

lahsivjar Apr 11, 2025

carsonip Apr 11, 2025 •

edited

Loading

lahsivjar Apr 11, 2025

lahsivjar left a comment

lahsivjar Apr 11, 2025

lahsivjar Apr 11, 2025

lahsivjar Apr 11, 2025

		// TODO(carsonip): use opentelemetry-collector-contrib/pkg/pdatautil/hash.go when it can optionally exclude attributes
		// We could have used it now but it'll involve creating a new Map and copying things over.

[exporter/elasticsearch] Handle edge cases in metrics grouping causing TSDB version_conflict_engine_exception #39309

Are you sure you want to change the base?

[exporter/elasticsearch] Handle edge cases in metrics grouping causing TSDB version_conflict_engine_exception #39309

Conversation

carsonip commented Apr 10, 2025 • edited Loading

Description

Link to tracking issue

Testing

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lahsivjar Apr 11, 2025 • edited Loading

Choose a reason for hiding this comment

lahsivjar Apr 11, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carsonip Apr 11, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carsonip Apr 11, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lahsivjar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carsonip commented Apr 10, 2025 •

edited

Loading

lahsivjar Apr 11, 2025 •

edited

Loading

lahsivjar Apr 11, 2025 •

edited

Loading

carsonip Apr 11, 2025 •

edited

Loading

carsonip Apr 11, 2025 •

edited

Loading