opensearch-project / data-prepper

Data Prepper is a component of the OpenSearch project that accepts, filters, transforms, enriches, and routes data at scale.
https://opensearch.org/docs/latest/clients/data-prepper/index/
Apache License 2.0
259 stars 190 forks source link

[BUG] update Metrics exemplar field schema #4647

Open YANG-DB opened 3 months ago

YANG-DB commented 3 months ago

Describe the bug here is the Otel spec we are expecting

        "exemplar": {
          "properties": {
            "time": {
              "type": "date"
            },
            "traceId": {
              "ignore_above": 256,
              "type": "keyword"
            },
            "serviceName": {
              "ignore_above": 256,
              "type": "keyword"
            },
            "spanId": {
              "ignore_above": 256,
              "type": "keyword"
            }
          }
        } 

which is based on the otel protocol definition

But this is the json currently being generated:

  "exemplars": [
    {
      "time": "2024-06-18T05:11:33.182223836Z",
      "value": 1,
      "attributes": {
        "clientip": "1.1.1.1",
        "id": "id1",
        "serviceName": "svc0",
        "traceId": "trace0",
        "spanId": "span1"
      },
      "spanId": null,
      "traceId": null
    }
  ],

Expected behavior 1) please rename exemplars to exemplar 2) populate the spanId, traceId in the root level 3) add serviceName field at the root level of the exemplar

Screenshots If applicable, add screenshots to help explain your problem.

Environment (please complete the following information):

Additional context Add any other context about the problem here.

kkondaka commented 3 months ago

@KarstenSchnitter @kaimst @dlvenable It looks like exemplars is not a list. As per the open telemetry specification, it is just one exemplar - https://github.com/open-telemetry/opentelemetry-proto/blob/bd7cf55b6d45f3c587d2131b68a7e5a501bdb10c/opentelemetry/proto/metrics/v1/metrics.proto#L660.

I think we should change withExemplars() API in JacksonMetric and its uses to be one Exemplar and not a list of Exemplars. What do you think?

KarstenSchnitter commented 3 months ago

The protobuf defintion of the OpenTelemetry metrics model clearly specifies the exmplars as a repeated field:

  // (Optional) List of exemplars collected from
  // measurements that were used to form the data point
  repeated Exemplar exemplars = 11;

https://github.com/open-telemetry/opentelemetry-proto/blob/bd7cf55b6d45f3c587d2131b68a7e5a501bdb10c/opentelemetry/proto/metrics/v1/metrics.proto#L574

As such, the array generated by DataPrepper looks correct to me.

kkondaka commented 3 months ago

@KarstenSchnitter interesting. It looks like for Summary Datapoint, only one exemplar and for histogram multiple exemplars. Should we modify JacksonMetric to support both withExemplars and withExemplar?

KarstenSchnitter commented 3 months ago

I can acutally not find a reference to an exemplar in the protobuf definitions for SummaryDataPoint or ValueAtQuantile. Maybe I am overlooking something. But the OpenTelemetry Metrics Data Model also does not contain such a reference. It explains the use-cases for exemplars though.

kkondaka commented 3 months ago

@KarstenSchnitter I have provided the link above ( https://github.com/open-telemetry/opentelemetry-proto/blob/bd7cf55b6d45f3c587d2131b68a7e5a501bdb10c/opentelemetry/proto/metrics/v1/metrics.proto#L660.) Is this not good?

KarstenSchnitter commented 3 months ago

@kkondaka you linked to the definition of a single Exemplar record. This needs to be referenced in the DataPoints like in my linke for the HistogramDataPoint. This reference is missing in the SummaryDataPoint. You can also see this in the generated classes in the sources jar of https://mvnrepository.com/artifact/io.opentelemetry.proto/opentelemetry-proto/1.3.1-alpha.

kkondaka commented 3 months ago

@KarstenSchnitter You are right. I got confused because right above it is SummaryDataPoint and I thought exemplar is part it. @YANG-DB As per the model definition, SummaryDataPoint can have multiple NumberDataPoint and each NumberDataPoint may have multiple exemplars. So, it looks like the current implementation is good

YANG-DB commented 3 months ago

@kkondaka @KarstenSchnitter thanks for the deep dive here - we will update the schema to reflect these findings...