Support trace groups in standard otel proto codec by kkondaka · Pull Request #5539 · opensearch-project/data-prepper

kkondaka · 2025-03-21T21:20:18Z

Description

Support OTEL trace groups and trace group fields in standaed OTEL proto codec
Cleanup and address comments from Add support for generating Standard(OTEL) conformant events in DataPrepper #5524
Add template mappings for using standard OTEL log/trace/metrics in opensearch sink

Issues Resolved

Resolves #5259

Check List

[X ] New functionality includes testing.
New functionality has a documentation issue. Please link to it in this PR.
- New functionality has javadoc added
[X ] 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.

kkondaka · 2025-03-21T21:20:30Z

@KarstenSchnitter @dlvenable please review

KarstenSchnitter

Thanks for providing this change.

I managed to review the logs and metrics template. Some of the comments on logs translate to metrics as well. I will review more of this PR over the weekend.

KarstenSchnitter · 2025-03-21T21:56:36Z

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

+              "type": "text",
+              "fields": {
+                "keyword": {
+                  "type": "keyword",
+                  "ignore_above": 128
+                }
+              }


I do not think, that it is necessary to do this kind of pattern on almost all of the OTel fields. They are usually very short to save space. instrumentationScope.name usually takes the form of a url, e.g. io.opentelemetry.runtime-telemetry-java8. I recommend using only the keyword field type. 128 characters also seems to be more than enough. Is there a particular reason to go for text as well?

KarstenSchnitter · 2025-03-21T21:57:21Z

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

+              "fields": {
+                "keyword": {
+                  "type": "keyword",
+                  "ignore_above": 256


This is a semantic version number. 256 seams way to long IMHO.

KarstenSchnitter · 2025-03-21T21:59:39Z

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

+          "path": "observedTimestamp"
+        },
+        "traceId": {
+          "ignore_above": 256,


This will always be 32 characters of a hex encoded 16 byte array.

KarstenSchnitter · 2025-03-21T21:59:59Z

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

+          "type": "keyword"
+        },
+        "spanId": {
+          "ignore_above": 256,


This will always be 16 characters of a hex encoded 8 byte array.

KarstenSchnitter · 2025-03-21T22:01:20Z

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

+          "type": "keyword"
+        },
+        "flags": {
+          "type": "integer"


This is a bit mask. I am not sure, whether there are better field types available.

...ns/opensearch/src/main/resources/index-template/metrics-otel-v1-index-standard-template.json

KarstenSchnitter · 2025-03-21T22:38:16Z

...ns/opensearch/src/main/resources/index-template/metrics-otel-v1-index-standard-template.json

+        },
+        "kind": {
+          "type": "keyword",
+          "ignore_above": 128


This is actually an enum with well-known values. The longest is EXPONENTIAL_HISTOGRAM.

...ns/opensearch/src/main/resources/index-template/metrics-otel-v1-index-standard-template.json

KarstenSchnitter · 2025-03-21T22:41:16Z

...ns/opensearch/src/main/resources/index-template/metrics-otel-v1-index-standard-template.json

+          "type": "date"
+        },
+        "@timestamp": {
+          "type": "date"


Why is this not date_nanos?

KarstenSchnitter · 2025-03-21T22:41:39Z

...ns/opensearch/src/main/resources/index-template/metrics-otel-v1-index-standard-template.json

+              "type": "double"
+            },
+            "max": {
+              "type": "float"


Why not use double?

KarstenSchnitter

I reviewed everything I left out in my last review. I haved worked quite a lot with mapping templates for OTel data. I can only recommend to use component templates to ease the definition. Resource attributes and Instrumentation Scope handling can be shared amongst the signals. It needs to be the same for efficient analysis anyways. We also should give users the option to extend the templates for covering semantic conventions on the attributes, e..g., specifying http.status.code to be an integer. I added a lot of coments on the mappings, which also apply to the other signals and sometimes other fields.

KarstenSchnitter · 2025-03-22T05:51:44Z

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

+          "resource_attributes_map": {
+            "mapping": {
+              "type": "keyword"
+            },
+            "path_match": "resource.attributes.*"
+          }
+        },
+        {
+          "span_attributes_map": {
+            "mapping": {
+              "type": "keyword"
+            },
+            "path_match": "span.attributes.*"
+          }
+        }
+      ],


Why do you porpose dynamic templates for span and resource attributes but not instrumentation scope? For logs and metrics there was a mapping to object. I like ethe dynamic templating better. But this should be uniform for all three signals and all three attribute collections.

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

KarstenSchnitter · 2025-03-22T05:55:47Z

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

+          }
+        },
+        "kind": {
+          "ignore_above": 128,


This is actually an enum with known values. The longest is SPAN_KIND_UNSPECIFIED.

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

KarstenSchnitter · 2025-03-22T05:57:56Z

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

+        "attributes": {
+          "type": "object"
+        },


This is semantically the same as the span attributes from the dynamic templates.

KarstenSchnitter · 2025-03-22T06:02:13Z

...ns/opensearch/src/main/resources/index-template/metrics-otel-v1-index-standard-template.json

+          "type": "text",
+          "fields": {
+            "keyword": {
+              "type": "keyword",
+              "ignore_above": 256
+            }
+          }
+        }


Just using a keyword should be sufficient. There are two schemaUrls though: One for resource attributes and one for instrumenation scope attributes. Logs and metrics are missing this entry entirely.

KarstenSchnitter · 2025-03-22T06:04:35Z

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

+          "ignore_above": 1024,
+          "type": "keyword"
+        },
+        "traceGroupFields": {


It would actually be nice to add (resource) attributes to the trace group in a future extension.

KarstenSchnitter · 2025-03-22T06:05:57Z

data-prepper-plugins/otel-logs-source/src/test/resources/testjson/test-standard-log.json

  },
  "flags": 0,
  "severityNumber": 2,
-  "serviceName": "testServiceName",


This should have stayed.

KarstenSchnitter · 2025-03-22T06:06:36Z

data-prepper-plugins/otel-metrics-source/src/test/resources/testjson/test-standard-metrics.json

    "kind": "GAUGE",
    "flags": 0,
    "description": "testGaugeDescription",
-    "serviceName": null,


THis should have stayed, but not using null which is somewhat invalid. All OTel resources SHOULD have a service name.

...mmon/src/main/java/org/opensearch/dataprepper/plugins/otel/codec/OTelProtoStandardCodec.java

kkondaka · 2025-03-25T00:03:19Z

@KarstenSchnitter thank you very much for the detailed review. Since you have experience, would it be possible to provide me the complete template files?

KarstenSchnitter · 2025-03-25T05:03:01Z

@KarstenSchnitter thank you very much for the detailed review. Since you have experience, would it be possible to provide me the complete template files?

Yes, I am going to work on it today.

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

@kkondaka please find my proposal in kkondaka#2.

KarstenSchnitter · 2025-04-05T20:37:33Z

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

+        {
+          "double_attributes": {
+            "mapping": {
+              "type": "double"
+            },
+            "path_match": "attributes.*",
+            "match_mapping_type": "double"
+          }
+        }


Can you add at least string attributs, if the default was not working? And similar for the other attributes.

Suggested change

{

"double_attributes": {

"mapping": {

"type": "double"

},

"path_match": "attributes.*",

"match_mapping_type": "double"

}

}

{

"double_attributes": {

"mapping": {

"type": "double"

},

"path_match": "attributes.*",

"match_mapping_type": "double"

}

},

{

"string_attributes": {

"mapping": {

"type": "keyword",

"ignore_above": 256

},

"path_match": "attributes.*",

"match_mapping_type": "string"

}

}

@KarstenSchnitter I have the change and verified that it works great. Please take a look at the diffs. Let's plan to make any other suggestions you may have in another PR

@kkondaka: Great, that it works. Can you add the string attributes for resource and instrumentation scope attributes as well? Then we should be good to go.

Signed-off-by: Krishna Kondaka <krishkdk@amazon.com>

…gs/traces/metrics Signed-off-by: Krishna Kondaka <krishkdk@amazon.com>

Signed-off-by: Krishna Kondaka <krishkdk@amazon.com>

KarstenSchnitter

The changed (non-standard) metrics template is now missing the string attributes. Please add them. Otherwise, this looks good.

KarstenSchnitter · 2025-04-07T19:29:04Z

...per-plugins/opensearch/src/main/resources/index-template/metrics-otel-v1-index-template.json

+            "path_match": "resource.attributes.*",
+            "match_mapping_type": "double"
+          }
+        },


This one is now missing the string attributes for resources, scope and plain.

Thanks @KarstenSchnitter. Will do a separate PR for this.

These are existing mappings. Not focusing on these in this PR, anyways.

dlvenable · 2025-04-07T20:48:40Z

.../src/test/java/org/opensearch/dataprepper/plugins/otel/codec/OTelProtoStandardCodecTest.java

            for (final Span span : spans) {
-                assertThat(span.getTraceGroup(), nullValue());
-                assertThat(span.getTraceGroupFields(), nullValue());
+                if (span.getParentSpanId().isEmpty()) {


This is a somewhat loose assertion. Do we even know if we ever get non-empty spans?

This is the exact same code that exists in OTelProtoOpensearchCodecTest. I did not include previously because there was not tracegroups support previously.

Yea, I see that you have expanded the scope of this test.

Could it be split into two different tests? One for each scenario?

@dlvenable The test data is coming from resources/test-request.json and it has two spans with parentSpan and one without parentSpan. So, I think this test is fine.

dlvenable · 2025-04-07T20:49:28Z

data-prepper-api/src/main/java/org/opensearch/dataprepper/model/trace/JacksonSpan.java

    private static final String DROPPED_LINKS_COUNT_KEY = "droppedLinksCount";
-    private static final String SERVICE_NAME_KEY = "serviceName";
-    private static final String TRACE_GROUP_KEY = "traceGroup";
+    public static final String SERVICE_NAME_KEY = "serviceName";


If you are using these in the tests, make them package protected. Remove public.

It needs to be public because it is used in data-prepper-plugins/otel-proto-common/src/main/java/org/opensearch/dataprepper/plugins/otel/codec/OTelProtoStandardCodec.java

dlvenable · 2025-04-07T20:52:32Z

...search/src/main/java/org/opensearch/dataprepper/plugins/sink/opensearch/index/IndexType.java


 public enum IndexType {
    TRACE_ANALYTICS_RAW("trace-analytics-raw"),
+    TRACE_ANALYTICS_RAW_STANDARD("trace-analytics-standard-raw"),


The term standard is unclear. Can you give a more precise adjective? Perhaps otel?

We can resolve in a follow-on PR.

…#5539) * Support trace groups in standard otel proto codec Signed-off-by: Krishna Kondaka <krishkdk@amazon.com> * Addressed review comments. Verified the functionality for standard logs/traces/metrics Signed-off-by: Krishna Kondaka <krishkdk@amazon.com> * Addressed review comments Signed-off-by: Krishna Kondaka <krishkdk@amazon.com> * Added string mappings for resource and scope Signed-off-by: Krishna Kondaka <krishkdk@amazon.com> --------- Signed-off-by: Krishna Kondaka <krishkdk@amazon.com>

…#5539) * Support trace groups in standard otel proto codec Signed-off-by: Krishna Kondaka <krishkdk@amazon.com> * Addressed review comments. Verified the functionality for standard logs/traces/metrics Signed-off-by: Krishna Kondaka <krishkdk@amazon.com> * Addressed review comments Signed-off-by: Krishna Kondaka <krishkdk@amazon.com> * Added string mappings for resource and scope Signed-off-by: Krishna Kondaka <krishkdk@amazon.com> --------- Signed-off-by: Krishna Kondaka <krishkdk@amazon.com> Signed-off-by: mamol27 <mamol27@yandex.ru>

kkondaka requested review from KarstenSchnitter, chenqi0805, dinujoh, dlvenable, engechas, graytaylor0, oeyh, san81, sb2k16 and srikanthjg as code owners March 21, 2025 21:20

KarstenSchnitter reviewed Mar 21, 2025

View reviewed changes

KarstenSchnitter requested changes Mar 22, 2025

View reviewed changes

KarstenSchnitter reviewed Apr 5, 2025

View reviewed changes

kkondaka added 3 commits April 7, 2025 04:20

Support trace groups in standard otel proto codec

ca7beae

Signed-off-by: Krishna Kondaka <krishkdk@amazon.com>

Addressed review comments. Verified the functionality for standard lo…

3ccda29

…gs/traces/metrics Signed-off-by: Krishna Kondaka <krishkdk@amazon.com>

Addressed review comments

076047f

Signed-off-by: Krishna Kondaka <krishkdk@amazon.com>

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

Added string mappings for resource and scope

698ae2a

Signed-off-by: Krishna Kondaka <krishkdk@amazon.com>

KarstenSchnitter approved these changes Apr 7, 2025

View reviewed changes

dlvenable requested changes Apr 7, 2025

View reviewed changes

dlvenable approved these changes Apr 7, 2025

View reviewed changes

kkondaka merged commit 6be0b97 into opensearch-project:main Apr 7, 2025
63 of 68 checks passed

KarstenSchnitter mentioned this pull request Apr 8, 2025

Add OTel Metrics String Attributes to Index Template #5589

Merged

4 tasks

kkondaka mentioned this pull request Apr 8, 2025

[BUG] Unsupported OTel metric, instrumentation scope and resource fields #4137

Closed

kkondaka deleted the otel-trace-groups branch July 1, 2025 17:04

Conversation

kkondaka commented Mar 21, 2025

Description

Issues Resolved

Check List

Uh oh!

kkondaka commented Mar 21, 2025

Uh oh!

KarstenSchnitter left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KarstenSchnitter left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kkondaka commented Mar 25, 2025

Uh oh!

KarstenSchnitter commented Mar 25, 2025

Uh oh!

KarstenSchnitter commented Mar 25, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KarstenSchnitter Apr 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KarstenSchnitter left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KarstenSchnitter Apr 7, 2025 •

edited

Loading