[elasticsearchexporter]: Add dynamic document id support for logs

[mauri870]

Description

This PR adds a new config option logs_dynamic_id that when set to true reads the elasticsearch.document_id attribute from each log record and uses it as the final document id in Elasticsearch. This is only implemented for logs but I can open subsequent PRs supporting metrics and traces akin to the *_dynamic_index options.

Fixes #36882

Testing

Added tests to verify that the document ID attribute can be read from the log record and that the _id is properly forwarded to Elasticsearch. Also asserted that when there is no doc id attribute the current behavior is retained.

Documentation

Updated the readme to mention the new logs_dynamic_id config option.

[carsonip]

looks good at a high level, thanks!

[carsonip]

Q: do you expect the elasticsearch.document_id to be indexed under attributes as well, or should it be ignored during serialization?

[mauri870]

Q: do you expect the elasticsearch.document_id to be indexed under attributes as well, or should it be ignored during serialization?

Haven't thought of that, but I think it makes sense to remove the field, as it only has meaning for the exporter.

Also, elasticsearch.document_id is overly specific, I'm happy to hear suggestions on the naming.

[mauri870]

I updated the code to remove the id field from the final document.

Regarding the attribute name, I wonder if we should stick to some kind of semantic convention such as https://opentelemetry.io/docs/specs/semconv/database/elasticsearch/. I scrolled throught it but couldn't find an existing attribute for a document id.

[axw]

LGTM, just a couple of nits

[codecov]

Codecov Report

Attention: Patch coverage is 91.66667% with 2 lines in your changes missing coverage. Please review.

Project coverage is 79.57%. Comparing base (37c8044) to head (490e747).
Report is 32 commits behind head on main.

Files with missing lines	Patch %	Lines
exporter/elasticsearchexporter/exporter.go	87.50%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #37065   +/-   ##
=======================================
  Coverage   79.57%   79.57%           
=======================================
  Files        2271     2271           
  Lines      212855   212872   +17     
=======================================
+ Hits       169372   169392   +20     
- Misses      37794    37797    +3     
+ Partials     5689     5683    -6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[songy23]

needs a rebase

[mauri870]

Interesting, I fixed the conflicts but now the tests fail with panic: invalid access to shared data. This seems to be due to m.RemoveIf here.

I believe this is because a PR yesterday changed MutatesData to false #37234.

What should be the course of action here? The idea of the attribute defined here is that we remove it from the final document.

[ChrsMark]

What should be the course of action here? The idea of the attribute defined here is that we remove it from the final document.

@felixbarny any suggestion here?

[felixbarny]

Instead of removing the attribute from the original LogRecord, just skip the attribute when serializing it.

opentelemetry-collector-contrib/exporter/elasticsearchexporter/pdata_serializer.go

Lines 297 to 308 in a151d16

	attributes.Range(func(k string, val pcommon.Value) bool {
	switch k {
	case dataStreamType, dataStreamDataset, dataStreamNamespace, mappingHintsAttrKey:
	return true
	}
	if isGeoAttribute(k, val) {
	return true
	}
	_ = v.OnKey(k)
	writeValue(v, val, stringifyMapValues)
	return true
	})

IIUC, this is mostly relevant for the bodymap mapping mode which doesn't add attributes to the document anyway.

[mauri870]

Instead of removing the attribute from the original LogRecord, just skip the attribute when serializing it.

That is clever, worked like a charm. Thanks!

[mauri870]

CI failure seems to be unrelated with my changes.