Skip to content

Otel trace groups #2

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -51,9 +51,9 @@ public class JacksonSpan extends JacksonEvent implements Span {
private static final String LINKS_KEY = "links";
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 TRACE_GROUP_KEY = "traceGroup";
private static final String DURATION_IN_NANOS_KEY = "durationInNanos";
private static final String TRACE_GROUP_FIELDS_KEY = "traceGroupFields";
public static final String TRACE_GROUP_FIELDS_KEY = "traceGroupFields";

private static final List<String> REQUIRED_KEYS = Arrays.asList(TRACE_GROUP_KEY);
protected static final List<String>
Expand Down Expand Up @@ -177,6 +177,10 @@ public Integer getDroppedLinksCount() {

@Override
public String getTraceGroup() {
EventMetadata metadata = getMetadata();
Object traceGroup = metadata.getAttribute(TRACE_GROUP_KEY);
if (traceGroup != null)
return (String)traceGroup;
return this.get(TRACE_GROUP_KEY, String.class);
}

Expand All @@ -187,6 +191,10 @@ public Long getDurationInNanos() {

@Override
public TraceGroupFields getTraceGroupFields() {
EventMetadata metadata = getMetadata();
Object traceGroupFields = metadata.getAttribute(TRACE_GROUP_FIELDS_KEY);
if (traceGroupFields != null)
return (TraceGroupFields)traceGroupFields;
return this.get(TRACE_GROUP_FIELDS_KEY, DefaultTraceGroupFields.class);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@

package org.opensearch.dataprepper.model.trace;

import org.opensearch.dataprepper.model.event.EventMetadata;
import com.fasterxml.jackson.core.JsonProcessingException;
import io.micrometer.core.instrument.util.IOUtils;
import com.fasterxml.jackson.core.type.TypeReference;
Expand All @@ -19,6 +20,7 @@
import org.json.JSONException;
import org.skyscreamer.jsonassert.JSONAssert;

import java.time.Instant;
import java.util.Arrays;
import java.util.Map;
import java.util.List;
Expand Down Expand Up @@ -86,6 +88,33 @@ public JacksonSpan createObjectUnderTest(Map<String, Object> attributes) {
.build();
}

@Test
@Override
public void testGetTraceGroup() {
jacksonSpan = createObjectUnderTest(TEST_ATTRIBUTES);
EventMetadata metadata = jacksonSpan.getMetadata();
String testTraceGroup = UUID.randomUUID().toString();
metadata.setAttribute(JacksonSpan.TRACE_GROUP_KEY, testTraceGroup);
final String traceGroup = jacksonSpan.getTraceGroup();
assertThat(traceGroup, is(equalTo(testTraceGroup)));
}

@Test
@Override
public void testGetTraceGroupFields() {
jacksonSpan = createObjectUnderTest(TEST_ATTRIBUTES);
DefaultTraceGroupFields testTraceGroupFields =
DefaultTraceGroupFields.builder()
.withDurationInNanos(10000L)
.withEndTime(Instant.now().toString())
.withStatusCode(10)
.build();
EventMetadata metadata = jacksonSpan.getMetadata();
metadata.setAttribute(JacksonSpan.TRACE_GROUP_FIELDS_KEY, testTraceGroupFields);
final TraceGroupFields traceGroupFields = jacksonSpan.getTraceGroupFields();
assertThat(traceGroupFields, is(equalTo(testTraceGroupFields)));
}

@Test
@Override
public void testToJsonStringAllParameters() throws JsonProcessingException, JSONException {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -381,12 +381,18 @@ private Map<String, Object> readIndexTemplate(final String templateFile, final I
InputStream s3TemplateFile = null;
if (indexType.equals(IndexType.TRACE_ANALYTICS_RAW)) {
templateURL = loadExistingTemplate(templateType, IndexConstants.RAW_DEFAULT_TEMPLATE_FILE);
} else if (indexType.equals(IndexType.TRACE_ANALYTICS_RAW_STANDARD)) {
templateURL = loadExistingTemplate(templateType, IndexConstants.RAW_STANDARD_TEMPLATE_FILE);
} else if (indexType.equals(IndexType.TRACE_ANALYTICS_SERVICE_MAP)) {
templateURL = loadExistingTemplate(templateType, IndexConstants.SERVICE_MAP_DEFAULT_TEMPLATE_FILE);
} else if (indexType.equals(IndexType.LOG_ANALYTICS)) {
templateURL = loadExistingTemplate(templateType, IndexConstants.LOGS_DEFAULT_TEMPLATE_FILE);
} else if (indexType.equals(IndexType.LOG_ANALYTICS_STANDARD)) {
templateURL = loadExistingTemplate(templateType, IndexConstants.LOGS_STANDARD_TEMPLATE_FILE);
} else if (indexType.equals(IndexType.METRIC_ANALYTICS)) {
templateURL = loadExistingTemplate(templateType, IndexConstants.METRICS_DEFAULT_TEMPLATE_FILE);
} else if (indexType.equals(IndexType.METRIC_ANALYTICS_STANDARD)) {
templateURL = loadExistingTemplate(templateType, IndexConstants.METRICS_STANDARD_TEMPLATE_FILE);
} else if (templateFile != null) {
if (templateFile.toLowerCase().startsWith(S3_PREFIX)) {
FileReader s3FileReader = new S3FileReader(s3Client);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,16 +15,19 @@ public class IndexConstants {
public static final Map<IndexType, String> TYPE_TO_DEFAULT_ALIAS = new HashMap<>();
// TODO: extract out version number into version enum
public static final String RAW_DEFAULT_TEMPLATE_FILE = "otel-v1-apm-span-index-template.json";
public static final String RAW_STANDARD_TEMPLATE_FILE = "otel-v1-apm-span-index-standard-template.json";
public static final String RAW_ISM_POLICY = "raw-span-policy";
public static final String RAW_ISM_FILE_NO_ISM_TEMPLATE = "raw-span-policy-no-ism-template.json";
public static final String RAW_ISM_FILE_WITH_ISM_TEMPLATE = "raw-span-policy-with-ism-template.json";

public static final String LOGS_DEFAULT_TEMPLATE_FILE = "logs-otel-v1-index-template.json";
public static final String LOGS_STANDARD_TEMPLATE_FILE = "logs-otel-v1-index-standard-template.json";
public static final String LOGS_ISM_POLICY = "logs-policy";
public static final String LOGS_ISM_FILE_NO_ISM_TEMPLATE = "logs-policy-no-ism-template.json";
public static final String LOGS_ISM_FILE_WITH_ISM_TEMPLATE = "logs-policy-with-ism-template.json";

public static final String METRICS_DEFAULT_TEMPLATE_FILE = "metrics-otel-v1-index-template.json";
public static final String METRICS_STANDARD_TEMPLATE_FILE = "metrics-otel-v1-index-standard-template.json";
public static final String METRICS_ISM_POLICY = "metrics-policy";
public static final String METRICS_ISM_FILE_NO_ISM_TEMPLATE = "metrics-policy-no-ism-template.json";
public static final String METRICS_ISM_FILE_WITH_ISM_TEMPLATE = "metrics-policy-with-ism-template.json";
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ public final IndexManager getIndexManager(final IndexType indexType,
IndexManager indexManager;
switch (indexType) {
case TRACE_ANALYTICS_RAW:
case TRACE_ANALYTICS_RAW_STANDARD:
indexManager = new TraceAnalyticsRawIndexManager(
restHighLevelClient, openSearchClient, openSearchSinkConfiguration, clusterSettingsParser, templateStrategy, indexAlias);
break;
Expand All @@ -59,10 +60,12 @@ public final IndexManager getIndexManager(final IndexType indexType,
restHighLevelClient, openSearchClient, openSearchSinkConfiguration, clusterSettingsParser, templateStrategy, indexAlias);
break;
case LOG_ANALYTICS:
case LOG_ANALYTICS_STANDARD:
indexManager = new LogAnalyticsIndexManager(
restHighLevelClient, openSearchClient, openSearchSinkConfiguration, clusterSettingsParser, templateStrategy, indexAlias);
break;
case METRIC_ANALYTICS:
case METRIC_ANALYTICS_STANDARD:
indexManager = new MetricAnalyticsIndexManager(
restHighLevelClient, openSearchClient, openSearchSinkConfiguration, clusterSettingsParser, templateStrategy, indexAlias);
break;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,9 +13,12 @@

public enum IndexType {
TRACE_ANALYTICS_RAW("trace-analytics-raw"),
TRACE_ANALYTICS_RAW_STANDARD("trace-analytics-standard-raw"),
TRACE_ANALYTICS_SERVICE_MAP("trace-analytics-service-map"),
LOG_ANALYTICS("log-analytics"),
LOG_ANALYTICS_STANDARD("log-analytics-standard"),
METRIC_ANALYTICS("metric-analytics"),
METRIC_ANALYTICS_STANDARD("metric-analytics-standard"),
CUSTOM("custom"),
MANAGEMENT_DISABLED("management_disabled");

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,167 @@
{
"version": 1,
"template": {
"mappings": {
"date_detection": false,
"_source": {
"enabled": true

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where does this limitation come from?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have we added this?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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"
      }
}

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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"
            }
          }
        },
        

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

"mapping": {
"type": "keyword",
"ignore_above": 256
},
"path_match": "resource.attributes.*"
}
},
{
"long_scope_attributes": {
"mapping": {
"type": "long"
},
"path_match": "instrumentationScope.attributes.*",
"match_mapping_type": "long"
}
},
{
"double_scope_attributes": {
"mapping": {
"type": "double"
},
"path_match": "instrumentationScope.attributes.*",
"match_mapping_type": "double"
}
},
{
"default_scope_attributes_map": {
"mapping": {
"type": "keyword",
"ignore_above": 256
},
"path_match": "instrumentationScope.attributes.*"
}
},
{
"long_attributes": {
"mapping": {
"type": "long"
},
"path_match": "attributes.*",
"match_mapping_type": "long"
}
},
{
"double_attributes": {
"mapping": {
"type": "double"
},
"path_match": "attributes.*",
"match_mapping_type": "double"
}
},
{
"default_attributes": {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

"mapping": {
"type": "keyword",
"ignore_above": 256
},
"path_match": "attributes.*"
}
}
],
"properties": {
"droppedAttributesCount": {
"type": "integer"
},
"instrumentationScope": {
"properties": {
"droppedAttributesCount": {
"type": "integer"
},
"schemaUrl": {
"type": "keyword",
"ignore_above": 256
},
"name": {
"type": "keyword",
"ignore_above": 128
},
"version": {
"type": "keyword",
"ignore_above": 64
}
}
},
"resource": {
"properties": {
"droppedAttributesCount": {
"type": "integer"
},
"schemaUrl": {
"type": "keyword",
"ignore_above": 256
}
}
},
"severity": {
"properties": {
"number": {
"type": "integer"
},
"text": {
"type": "keyword",
"ignore_above": "32"
}
}
},
"body": {
"type": "text"
},
"@timestamp": {
"type": "date_nanos"
},
"time": {
"type": "date_nanos"
},
"observedTime": {
"path": "date_nanos"
},
"traceId": {
"type": "keyword",
"ignore_above": 32
},
"spanId": {
"type": "keyword",
"ignore_above": 16

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

},
"flags": {
"type": "long"

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I renamed all the droppedEventsCount to droppedAttributesCount.

},
"serviceName": {
"type": "alias",
"path": "resource.attributes.service.name"
}
}
}
}
}
Loading
Loading