Otel trace groups #2

KarstenSchnitter · 2025-03-25T14:29:03Z

Description

Adjustment of the mapping templates for the standard OTel format.

Check List

Commits are signed with a real name per the DCO

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Signed-off-by: Krishna Kondaka <[email protected]>

Provide dynamic templates for all attributes. Adds `ignore_above` to all key word fields. Use reasonable defaults for OTel data. Signed-off-by: Karsten Schnitter <[email protected]>

ps48 · 2025-03-26T17:10:31Z

...s/opensearch/src/main/resources/index-template/otel-v1-apm-span-index-standard-template.json

      "dynamic_templates": [
        {
-          "resource_attributes_map": {
+          "integer_resource_attributes": {


Should this be named as long_resource_attributes and similarly below as well

ps48 · 2025-03-26T17:28:01Z

...ugins/opensearch/src/main/resources/index-template/logs-otel-v1-index-standard-template.json

+            "droppedEventsCount": {
+              "type": "integer"


Should this be droppedAttributesCount based on the OTel proto: https://github.com/open-telemetry/opentelemetry-proto/blob/d7770822d70c7bd47a6891fc9faacc66fc4af3d3/opentelemetry/proto/common/v1/common.proto#L80?

Integer is at 32bits but signed. The proto definition specifies an unsigned 32bit integer. Do you think this difference is significant? I find it hard to imagine there would be a scenario where that number of attributes is ever attached and dropped to a log/metric/span.

You are just referring to the naming, right? I am going to correct that.

vamsimanohar · 2025-03-26T21:17:33Z

...ugins/opensearch/src/main/resources/index-template/logs-otel-v1-index-standard-template.json

@@ -6,58 +6,129 @@
      "_source": {
        "enabled": true
      },
+      "dynamic_templates": [
+        {
+          "integer_resource_attributes": {


shouldn't this be long_resource_attributes
I think the question is already raised in other comments. can we address that in every signal type.

vamsimanohar · 2025-03-26T21:20:13Z

...ugins/opensearch/src/main/resources/index-template/logs-otel-v1-index-standard-template.json

-             "droppedEventsCount": {
-               "type": "integer"
-             }
+            "droppedEventsCount": {


shouldn't this be droppedEventsCount?

I am confused. What do want to change? This is the name of the field. Only indentation seems to be off.

I am going to change this to droppedAttributesCount.

vamsimanohar · 2025-03-26T21:23:18Z

...ugins/opensearch/src/main/resources/index-template/logs-otel-v1-index-standard-template.json

-          "ignore_above": 256,
-          "type": "keyword"
+          "type": "keyword",
+          "ignore_above": 16


Why are we limiting to 16 chars here and 32 in traceId?

First, this is not a limitation of storage, just of the indexing. W3C Trace Context specifies the trace id to be an array of 16 bytes and the span id to be an array of 8 bytes. With hex-encoding this produces 32 and 16 characters respectively. Using B3 context would provide the same length. So it seems to be a reasonable choice.

vamsimanohar · 2025-03-26T21:25:02Z

...ugins/opensearch/src/main/resources/index-template/logs-otel-v1-index-standard-template.json

-          }
+        "serviceName": {
+          "type": "alias",
+          "path": "resource.attributes.service.name"
        },
        "event": {


Why do we need event here?
https://github.com/open-telemetry/opentelemetry-proto/blob/main/opentelemetry/proto/logs/v1/logs.proto
We don't have anything in logs proto model.

That is a question I am wondering myself. This comes out of the Simple Schema for Observability. I kept it, since it was in the original file proposed by @kkondaka. I know this is not in the OpenTelemetry standard. The same is true for the data_stream. How do we want to go about this?

I will remove this.

Ok, I take it out in my PR to your branch.

vamsimanohar · 2025-03-26T21:28:05Z

...ugins/opensearch/src/main/resources/index-template/logs-otel-v1-index-standard-template.json

@@ -6,58 +6,129 @@
      "_source": {
        "enabled": true


can you also add eventName which is missing: https://github.com/open-telemetry/opentelemetry-proto/blob/main/opentelemetry/proto/logs/v1/logs.proto#L226

We have done some analysis on logs mapping here: opensearch-project/opensearch-catalog#217

Will add the field. I am going to take a look at the analysis.

@KarstenSchnitter It looks like we need to bump up the version number to 3 (at least for spans). Otherwise new template mappings are not being sent to the OpenSearch server.

Where does this limitation come from?

Have we added this?

Also, this could look much nicer, if I could use component templates. That way, I could use the same dynamic mappings on all three templates. Is there a way to do that with Data Prepper?

vamsimanohar · 2025-03-26T21:29:50Z

...ugins/opensearch/src/main/resources/index-template/logs-otel-v1-index-standard-template.json

        },
        "flags": {
-          "type": "integer"
+          "type": "long"


https://github.com/open-telemetry/opentelemetry-proto/blob/main/opentelemetry/proto/logs/v1/logs.proto#L178
Can we also add dropped_attributes_count in log

This is defined as fixed32. Since this is interpreted as a bit field, I decided to use long to not change representation, when the highest bit is set. Do you prefer using integer, since only the lowest bits are currently in use?

I am going to check, that for very attribute group there is a dropped attributes count. Thanks for pointing this out.

I renamed all the droppedEventsCount to droppedAttributesCount.

vamsimanohar · 2025-03-26T21:31:13Z

...ugins/opensearch/src/main/resources/index-template/logs-otel-v1-index-standard-template.json

+                      "type": "keyword",
+                      "ignore_above": 2048
+                    }
+                  }
                },
                "stacktrace": {
                  "type": "text"


``` "@timestamp": { "type": "date_nanos" }, "time": { "type": "date_nanos" }, "observedTimestamp": { "type": "date_nanos" }, "observedTime": { "type": "alias", "path": "observedTimestamp" }, ``` We have four fields related to timestamp. We have two in otel proto files, can we consolidate? If they are required why?

This was in the original file. The OTel specification only guarantees, that at least one of time and observedTime is set. It is not telling which one. I think, @timestamp is supposed to always contain a timestamp for each message. Furthermore, it provides a common timestamp field for all three signals. The alias seems to be there for compatibility.

I think @timestamp is opensearch specific. Correct me if I am wrong. And observedTime is incorrect. I think observedTimestamp is correct. I do not think my new code generates "@timestamp" and "observedTime". The mappings have "observedTime" as alias for "observedTimestamp", which may be removed but I think it's ok to keep it.

Time vs Timestamp is essentially OpenTelemetry proto-files vs Logs Data Model. I am fine with either naming as long as it is consistent.

vamsimanohar · 2025-03-26T21:50:22Z

...s/opensearch/src/main/resources/index-template/otel-v1-apm-span-index-standard-template.json

      "properties": {
+        "droppedEventsCount": {


dropperAttributesCount. Also can you validate the existing mapping with
https://github.com/open-telemetry/opentelemetry-proto/blob/main/opentelemetry/proto/trace/v1/trace.proto

I see a lot of missing pieces.

we did some analysis here: opensearch-project/opensearch-catalog#218

I am going to do that.

@timestamp

- renaming of droppedEventsCount to droppedAttributesCount - streamlined timestamps to include OTel proto defined times and on @timestamp for index ordering in OS - removed datastream attributes which are covered by dynamic template anyways - removed schemaUrl from logs and metrics but kept in scope and resource - added details on exemplars, links and events Signed-off-by: Karsten Schnitter <[email protected]>

KarstenSchnitter · 2025-03-27T14:49:53Z

I addressed the comments by @ps48 and @vamsimanohar. There are some open questions:

What do we do about the events in the logs model? This is not part of the OTel Logs Data Model but in sso4. I kept the mappings, since they do not hurt when there is no data.
Timestamp handling: I kept a common @timestamp field in logs, metrics and traces. The idea is to have a clear timestamp to align on and ease analysis across the signals. For logs @timestamp should contain the optional time and observedTime if it is not available. For metrics @timestamp should contain the time. We can make it an alias, but for index patterns it would be better to be at least a copy_to. Since it needs custom logic in the logging case, it is not an option there.

Signed-off-by: Karsten Schnitter <[email protected]>

kkondaka · 2025-04-03T06:23:34Z

...ugins/opensearch/src/main/resources/index-template/logs-otel-v1-index-standard-template.json

+          }
+        },
+        {
+          "default_resource_attributes": {


@KarstenSchnitter Is this correct? Default attributes can be objects also, right? It looks like, here you are assuming them to be strings. I think it is possible for attributes to be nested like

"resource": { "attributes" : { "data" : { "key1": "value1" "key2": 2 }, "key3": "value3" } }

Due to the path_match this is applied to all nested fields. With the three dynamic mappings your example would have the values of key1 indexed as keyword, of key2 as log, and of key3 as keyword.

@KarstenSchnitter I am seeing this error with the mappings you provided -

WARN org.opensearch.dataprepper.plugins.sink.opensearch.BulkRetryStrategy - index = otel-v1-apm-span-000001, operation = Index, status = 400, error = object mapping for [resource.attributes.host.image.id] tried to parse field [host.image.id] as object, but found a concrete value

Any idea why this error is occurring?

The mappings from the server

dynamic_templates" : [ { "long_resource_attributes" : { "path_match" : "resource.attributes.*", "mapping" : { "type" : "long" }, "match_mapping_type" : "long" } }, { "double_resource_attributes" : { "path_match" : "resource.attributes.*", "mapping" : { "type" : "double" }, "match_mapping_type" : "double" } }, { "default_resource_attributes" : { "path_match" : "resource.attributes.*", "mapping" : { "type" : "object" } } },

I have deleted the "default_resource_attributes", "default_scope_attributes" and "default_attributes" and then I was able to store the data and see the data using opensearch dashboards.

This looks like a typical type mismatch in the error message. Note, that the default_resource_attributes is not what I provided. It should have type keyword. Do you have a procedure to reproduce this issue?

@KarstenSchnitter sorry, that was copy-paste mistake. Even with the correct mappings with "keyword", I am still seeing the error. For now, I am going to take out the default attributes section in all three and updated the PR. These changes are working fine for traces,logs and metrics. Please submit another PR separately (on top of this) for any changes you want to make after I merge these change.

Please do not take it out. Mapping to keywords is the essential default we need. Can you try with an additional "match_mapping_type": "string" instead? I have not seen this problem during my testing. Can you provide the events you were testing with?

kkondaka · 2025-04-03T06:24:15Z

...ugins/opensearch/src/main/resources/index-template/logs-otel-v1-index-standard-template.json

+          }
+        },
+        {
+          "default_attributes": {


Same comment here. General attributes may also have nested attributes, right?

kkondaka · 2025-04-03T06:25:23Z

...ugins/opensearch/src/main/resources/index-template/logs-otel-v1-index-standard-template.json

-          }
+        "serviceName": {
+          "type": "alias",
+          "path": "resource.attributes.service.name"
        },
        "event": {


I will remove this.

kkondaka · 2025-04-03T06:32:14Z

...ugins/opensearch/src/main/resources/index-template/logs-otel-v1-index-standard-template.json

+                      "type": "keyword",
+                      "ignore_above": 2048
+                    }
+                  }
                },
                "stacktrace": {
                  "type": "text"


I think @timestamp is opensearch specific. Correct me if I am wrong. And observedTime is incorrect. I think observedTimestamp is correct. I do not think my new code generates "@timestamp" and "observedTime". The mappings have "observedTime" as alias for "observedTimestamp", which may be removed but I think it's ok to keep it.

kkondaka · 2025-04-03T07:07:06Z

...ugins/opensearch/src/main/resources/index-template/logs-otel-v1-index-standard-template.json

@@ -6,58 +6,129 @@
      "_source": {
        "enabled": true


@KarstenSchnitter It looks like we need to bump up the version number to 3 (at least for spans). Otherwise new template mappings are not being sent to the OpenSearch server.

Events are not part of the OTel Logs Data Model [1]. The mapping for the SS4O fields has been removed. [1] https://opentelemetry.io/docs/specs/otel/logs/data-model/ Signed-off-by: Karsten Schnitter <[email protected]>

KarstenSchnitter

I removed the event field from the logs mapping and answered some of the review comments. Timestamp names remain an open discussion.

KarstenSchnitter · 2025-04-03T08:56:13Z

...ugins/opensearch/src/main/resources/index-template/logs-otel-v1-index-standard-template.json

+          }
+        },
+        {
+          "default_resource_attributes": {


Due to the path_match this is applied to all nested fields. With the three dynamic mappings your example would have the values of key1 indexed as keyword, of key2 as log, and of key3 as keyword.

KarstenSchnitter · 2025-04-03T08:56:52Z

...ugins/opensearch/src/main/resources/index-template/logs-otel-v1-index-standard-template.json

-          }
+        "serviceName": {
+          "type": "alias",
+          "path": "resource.attributes.service.name"
        },
        "event": {


Ok, I take it out in my PR to your branch.

KarstenSchnitter · 2025-04-03T08:59:43Z

...ugins/opensearch/src/main/resources/index-template/logs-otel-v1-index-standard-template.json

+                      "type": "keyword",
+                      "ignore_above": 2048
+                    }
+                  }
                },
                "stacktrace": {
                  "type": "text"


Time vs Timestamp is essentially OpenTelemetry proto-files vs Logs Data Model. I am fine with either naming as long as it is consistent.

KarstenSchnitter · 2025-04-03T09:01:33Z

...ugins/opensearch/src/main/resources/index-template/logs-otel-v1-index-standard-template.json

        },
        "flags": {
-          "type": "integer"
+          "type": "long"


I renamed all the droppedEventsCount to droppedAttributesCount.

KarstenSchnitter · 2025-04-03T09:19:10Z

...ugins/opensearch/src/main/resources/index-template/logs-otel-v1-index-standard-template.json

@@ -6,58 +6,129 @@
      "_source": {
        "enabled": true


Where does this limitation come from?

kkondaka · 2025-04-05T19:06:05Z

@KarstenSchnitter ignore my comment about version being 3. Will keep it as 1. If there is an existing template with version 1 already present, it needs to be removed. Otherwise, we have to bump this version 2.

kkondaka and others added 4 commits March 21, 2025 21:17

Support trace groups in standard otel proto codec

f4d592b

Signed-off-by: Krishna Kondaka <[email protected]>

Adjust OTel Standard Logging Template

1d421b9

Provide dynamic templates for all attributes. Adds `ignore_above` to all key word fields. Use reasonable defaults for OTel data. Signed-off-by: Karsten Schnitter <[email protected]>

Adjust OTel Standard Span Template

89a0800

Provide dynamic templates for all attributes. Adds `ignore_above` to all key word fields. Use reasonable defaults for OTel data. Signed-off-by: Karsten Schnitter <[email protected]>

Adjust OTel Standard Metrics Template

e3051b6

Provide dynamic templates for all attributes. Adds `ignore_above` to all key word fields. Use reasonable defaults for OTel data. Signed-off-by: Karsten Schnitter <[email protected]>

KarstenSchnitter requested review from san81, srikanthjg and kkondaka as code owners March 25, 2025 14:29

KarstenSchnitter mentioned this pull request Mar 25, 2025

Support trace groups in standard otel proto codec opensearch-project/data-prepper#5539

Merged

2 tasks

ps48 reviewed Mar 26, 2025

View reviewed changes

vamsimanohar reviewed Mar 26, 2025

View reviewed changes

KarstenSchnitter requested review from vamsimanohar and ps48 March 27, 2025 14:50

FIx dynamic templates on signal attributes

e040262

Signed-off-by: Karsten Schnitter <[email protected]>

kkondaka reviewed Apr 3, 2025

View reviewed changes

Remove Event properties from OTel Log Template

d644858

Events are not part of the OTel Logs Data Model [1]. The mapping for the SS4O fields has been removed. [1] https://opentelemetry.io/docs/specs/otel/logs/data-model/ Signed-off-by: Karsten Schnitter <[email protected]>

KarstenSchnitter commented Apr 3, 2025

View reviewed changes

KarstenSchnitter requested a review from kkondaka April 3, 2025 09:20

kkondaka force-pushed the otel-trace-groups branch from ecdd704 to 076047f Compare April 7, 2025 04:20

Otel trace groups #2

Are you sure you want to change the base?

Otel trace groups #2

Conversation

KarstenSchnitter commented Mar 25, 2025

Description

Check List

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

vamsimanohar Mar 26, 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

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

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

vamsimanohar Mar 26, 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

KarstenSchnitter commented Mar 27, 2025

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

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

KarstenSchnitter 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kkondaka commented Apr 5, 2025

vamsimanohar Mar 26, 2025 •

edited

Loading

vamsimanohar Mar 26, 2025 •

edited

Loading