Respect data_stream.{dataset,namespace} for OTel logs, metrics, and traces #201

carsonip · 2024-01-15T17:06:23Z

Parse data_stream.{dataset,namespace} from OTel logs, metrics and traces
Route using user-defined data_stream.{dataset,namespace}

Implications / Breaking changes

data_stream.dataset and data_stream.namespace are parsed in the order of resource attributes, scope attributes, span attributes, and span events attributes. The latest match wins. e.g. a span event data_stream.dataset will override the span's attributes.
Implied by the previous point, data_stream.dataset and data_stream.namespace are inherited from earlier levels if they are not specified at the current level. e.g. if a span does not specify data_stream.dataset, it will use the scope's data_stream.dataset.
data_stream.dataset and data_stream.namespace are parsed and no longer set as labels (data_stream_dataset and data_stream_namespace)

Edge cases

Although data_stream.dataset and data_stream.namespace are parsed for error span events, only data_stream.namespace will be respected, because data_stream.dataset will be overridden by SetDataStream.
For RUM events, although data_stream.dataset and data_stream.namespace are parsed, none of them will be respected as they will be overridden by SetDataStream.

Closes elastic/apm-server#10191

joshdover · 2024-01-16T09:49:22Z

input/otlp/logs.go

@@ -182,6 +182,16 @@ func (c *Consumer) convertLogRecord(
 				event.Network.Connection = modelpb.NetworkConnectionFromVTPool()
 			}
 			event.Network.Connection.Type = v.Str()
+		case attributeDataStreamDataset:
+			if event.DataStream == nil {
+				event.DataStream = modelpb.DataStreamFromVTPool()


Will this respect OTLP Resource.attributes and LogRecord.attributes or only one? What about ScopeLogs.Scope.attributes?

Good call. It was only reading LogRecord.attributes, but I've updated the PR to only Resource.attributes (only one). Looking at the code, we never read from ScopeLogs.Scope.attributes. Let me know if it makes more sense to read from both LogRecord.attributes and Resource.attributes

This is currently unspecified in https://github.com/elastic/opentelemetry-dev/blob/main/docs/design-decisions/ingest/routing.md and I think we need to clarify this.

From an OTel Collector perspective, I think if we're only to support one, Scope.attributes would actually match the best. Reason being is that it's typical for a related dataset to emit a distinct Scope, which may come from the same Resource as other instrumentation. For example you may have two scopes, otelcol/hostmetricsreceiver/cpu and otelcol/hostmetricsreceiver/memory, which should be indexed into separate data streams coming from the same resource (host). Similarly you could have logs from two different application SDKs (say nginx and custom java app) from the same host, but should end up in different data streams for easier anaylsis and more compact storage.

That said, we could support all 3 levels with some precedence rules (I would expect LogRecord.attributes > Scope.attributes > Resource.attributes)

@axw have you given this any thought?

I'm not certain than any of them will cover all scenarios, e.g.:

resource attributes aren't appropriate for routing two different data sets produced by one data source, since resource describes the data source

instrumentation scope attributes aren't appropriate for routing data for two different service instances, since they would have identical instrumentation, just different resource attributes (e.g. service.instance.id)

I'm not sure that signal-level attributes are ever appropriate?

That said, we could support all 3 levels with some precedence rules (I would expect LogRecord.attributes > Scope.attributes > Resource.attributes)

Sounds good, for consistency with the attribute precedence we've already documented (sans top-level fields).

support all 3 levels with some precedence rules (I would expect LogRecord.attributes > Scope.attributes > Resource.attributes)

done

That said, we could support all 3 levels with some precedence rules (I would expect LogRecord.attributes > Scope.attributes > Resource.attributes)

I'm revisiting the docs and I'm a little confused here. The attribute precedence described by @joshdover (which I implemented in the PR atm) appears to be in the opposite order to our documented vision.

joshdover · 2024-01-16T09:49:28Z

How well is the existing document schema for these logs indexed into ES when they target data streams that use the default logs-*-* template instead of the logs-apm.app.*-* one?

Note that depending on the discussion in https://github.com/elastic/opentelemetry-dev/pull/59, we may need to append/prepend a otel suffix/prefix to the dataset if the one on the event isn't specified. I think though it would make sense to delay that until we implement https://github.com/elastic/observability-dev/issues/2986 and have new index templates that target these otel data streams.

carsonip · 2024-01-16T16:23:27Z

How well is the existing document schema for these logs indexed into ES when they target data streams that use the default logs-- template instead of the logs-apm.app.- one?

With a document as simple as

{
  "_index": ".ds-logs-apm.app.unknown_service-default-2024.01.16-000001",
  "_id": "6igPE40B0Q8GnOZbvKyd",
  "_version": 1,
  "_score": 0,
  "_source": {
    "observer": {
      "hostname": "carson-elastic",
      "type": "apm-server",
      "version": "8.13.0"
    },
    "agent": {
      "name": "otlp",
      "version": "unknown"
    },
    "@timestamp": "2024-01-16T16:16:53.948Z",
    "log": {
      "level": "fatal"
    },
    "data_stream": {
      "namespace": "default",
      "type": "logs",
      "dataset": "apm.app.unknown_service"
    },
    "service": {
      "name": "unknown_service",
      "language": {
        "name": "unknown"
      }
    },
    "event": {
      "severity": 21
    },
    "message": "test record",
    "labels": {
      "foo": "bar"
    }
  },
  "fields": {
    "event.severity": [
      21
    ],
    "service.name": [
      "unknown_service"
    ],
    "data_stream.namespace": [
      "default"
    ],
    "message": [
      "test record"
    ],
    "data_stream.type": [
      "logs"
    ],
    "observer.hostname": [
      "carson-elastic"
    ],
    "service.language.name": [
      "unknown"
    ],
    "@timestamp": [
      "2024-01-16T16:16:53.948Z"
    ],
    "observer.type": [
      "apm-server"
    ],
    "observer.version": [
      "8.13.0"
    ],
    "data_stream.dataset": [
      "apm.app.unknown_service"
    ],
    "log.level": [
      "fatal"
    ],
    "agent.name": [
      "otlp"
    ],
    "agent.version": [
      "unknown"
    ],
    "labels.foo": [
      "bar"
    ]
  }
}

They look virtually the same in logs-generic-default vs logs-apm*, at least in Discover. There are subtle differences in the underlying mapping.

logs-apm-mapping.json
logs-generic-mapping.json

Dotted fields are de-dotted, e.g. dot.ted becomes labels.dot_ted in either case and are working well.

axw

Looks good! Just one request. I wonder if we should do it for traces and metrics already, given that this is new functionality and won't affect anything unless these attributes are set.

input/otlp/logs.go

carsonip · 2024-01-22T20:08:01Z

@axw I've updated the PR to do the same for logs, metrics and traces. ~~What isn't handled is Consumer.convertSpanEvent which are errors and logs from spans.~~

carsonip · 2024-01-23T17:00:58Z

model/modelprocessor/datastream.go

+			// a different datastream.
+			if isRUMAgentName(event.GetAgent().GetName()) {
+				event.DataStream.Dataset = rumTracesDataset
+			}
 		}
 	case modelpb.ErrorEventType:


[For reviewer] Errors are not handled in a sense that the dataset is not respected. apm.error will always be used but the namespace is set by input data_stream.namespace.

carsonip · 2024-01-23T17:01:28Z

model/modelprocessor/datastream.go

+			event.DataStream.Dataset = tracesDataset
+			// In order to maintain different ILM policies, RUM traces are sent to
+			// a different datastream.
+			if isRUMAgentName(event.GetAgent().GetName()) {


[For reviewer] RUM check is bypassed if DataStream is specified. Is this desirable?

This reminded me: RUM agents shouldn't be control routing until we have some way for operators to restrict them. Otherwise (unauthenticated, untrusted) RUM agents can create arbitrarily many data streams, and DoS Elasticsearch.

carsonip · 2024-01-23T17:11:25Z

input/otlp/traces.go

[For reviewer] QQ: line 222, 223 otelSpan.Events() and event = event.CloneVT() means that errors and logs from traces automatically inherit data_stream.* from resource, scope and span.

For errors, in the PR we currently always override dataset to apm.error, but namespace will still be set using the input's data_stream.namespace. Since errors are special, so it makes sense to override dataset to apm.error in SetDataStream, but should it be consistent and override namespace as well?

For logs from traces, both dataset and namespace will be set by input's data_stream.*. It makes it indistinguishable from plain otel logs. Is this desirable?

Also, we are not reading data_stream.* inside convertSpanEvent. Should we do so as well?

I think we should also take it from the span event attributes, in case there's a need to override the span-level attributes.

we should also take it from the span event attributes

Q: span event including errors? Does it mean that we are allowing users to override apm.error dataset?

Q: what about events from jaeger? Should we parse data stream from attributes as well?

Q: span event including errors? Does it mean that we are allowing users to override apm.error dataset?

🥲 probably shouldn't allow overriding the dataset for now. Maybe later, if we can stop relying on the dataset for identifying errors. I expect we'll end up searching all logs for errors, but too early to say yet.

Q: what about events from jaeger? Should we parse data stream from attributes as well?

I don't think it matters a great deal - IMO whatever option avoids having a special case for Jaeger.

probably shouldn't allow overriding the dataset for now

Agreed. Dataset will be parsed at OTel conversion layer but later reset to apm.error by SetDataStream. However, data_stream.namespace will be parsed and respected. Not sure if you consider this a bug.

IMO whatever option avoids having a special case for Jaeger.

Although it means a few more lines, I think it is good to be consistent and do the same for Jaeger.

Agreed. Dataset will be parsed at OTel conversion layer but later reset to apm.error by SetDataStream. However, data_stream.namespace will be parsed and respected. Not sure if you consider this a bug.

I do, we should not allow untrusted agents to control data stream naming at all.

Although it means a few more lines, I think it is good to be consistent and do the same for Jaeger.

SGTM

I do, we should not allow untrusted agents to control data stream naming at all.

Which I see you have implemented, so LGTM

carsonip · 2024-01-23T17:17:12Z

we may need to append/prepend a otel suffix/prefix to the dataset if the one on the event isn't specified. I think though it would make sense to delay that until we implement https://github.com/elastic/observability-dev/issues/2986 and have new index templates that target these otel data streams.

Yes, it is a separate task to switch default dataset and possibly handle otel and non-otel cases separately. We can delay that. It should not block this PR.

carsonip · 2024-01-25T13:55:00Z

@axw sorry for dragging you into all these low level talk. I've updated the PR description to list the implications / breaking changes and edge case handling. See if it looks good to you.

axw

@carsonip I've reviewed the description and skimmed the code, changes LGTM. I think the order that Josh described sounds right, so you're that right it's not the same order as for queries.

I haven't reviewed all the code, let me know if I need to.

See elastic/apm-data#201

carsonip added 3 commits January 15, 2024 16:59

Parse data_stream.{dataset,namespace} from OTel logs

56bf536

Route using user-defined data_stream.{dataset,namespace} for logs

eed60d5

Better comment

37f7909

joshdover reviewed Jan 16, 2024

View reviewed changes

carsonip added 2 commits January 16, 2024 10:49

Use resource attribute data_stream.*

1f133b0

Remove log record attribute data_stream.*

4695926

Support 3 levels of data_stream.* with precedence

972a634

carsonip marked this pull request as ready for review January 17, 2024 17:05

carsonip requested a review from a team as a code owner January 17, 2024 17:05

carsonip requested review from axw and joshdover January 17, 2024 17:05

axw requested changes Jan 18, 2024

View reviewed changes

input/otlp/logs.go Outdated Show resolved Hide resolved

carsonip requested a review from lahsivjar January 18, 2024 10:18

carsonip added 3 commits January 18, 2024 11:57

Support data stream routing for otel metrics

7ae266b

Refactor logs_test

4b15844

Support data stream routing for otel traces

cc45742

carsonip changed the title ~~Respect data_stream.namespace and data_stream.dataset for OTel logs~~ Respect data_stream.{namespace,dataset} for OTel logs, metrics, and traces Jan 22, 2024

carsonip changed the title ~~Respect data_stream.{namespace,dataset} for OTel logs, metrics, and traces~~ Respect data_stream.{dataset,namespace} for OTel logs, metrics, and traces Jan 22, 2024

Revert convertLogRecord rename

356a7ee

carsonip requested a review from axw January 23, 2024 10:30

Update SetDataStream to respect traces and metrics DataStream

3473528

carsonip commented Jan 23, 2024

View reviewed changes

carsonip added 4 commits January 24, 2024 09:59

Disallow configurable data streams for RUM

9c48095

Override RUM data stream fields

14781c7

Read span event data_stream.*

664d9de

error: inconfigurable dataset & configurable namespace

b12a828

Handle jaeger errors

e24022f

axw approved these changes Jan 29, 2024

View reviewed changes

Merge branch 'main' into otel-respect-data-stream

f86266a

carsonip merged commit 3b28495 into elastic:main Feb 6, 2024
4 checks passed

carsonip added a commit to carsonip/apm-server that referenced this pull request Feb 6, 2024

Bump apm-data for OTel data_stream routing

3ac43ac

See elastic/apm-data#201

carsonip mentioned this pull request Feb 22, 2024

Bump apm-data for OTel data_stream routing elastic/apm-server#12578

Merged

2 tasks

simitt mentioned this pull request Mar 20, 2024

APM Server 8.13.0 Test Plan elastic/apm-server#12822

Closed

11 tasks

carsonip mentioned this pull request Jul 29, 2024

OTel data stream routing does not sanitize values elastic/apm-server#14043

Closed

ramdaspotale mentioned this pull request Sep 25, 2024

Instrument Elasticsearch with APM elastic/elasticsearch#84369

Closed

21 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Respect data_stream.{dataset,namespace} for OTel logs, metrics, and traces #201

Respect data_stream.{dataset,namespace} for OTel logs, metrics, and traces #201

carsonip commented Jan 15, 2024 •

edited

Loading

joshdover Jan 16, 2024

carsonip Jan 16, 2024 •

edited

Loading

joshdover Jan 16, 2024

axw Jan 17, 2024

carsonip Jan 17, 2024

carsonip Jan 25, 2024

joshdover commented Jan 16, 2024 •

edited by felixbarny

Loading

carsonip commented Jan 16, 2024

axw left a comment

carsonip commented Jan 22, 2024 •

edited

Loading

carsonip Jan 23, 2024

carsonip Jan 23, 2024

axw Jan 24, 2024

carsonip Jan 23, 2024

carsonip Jan 23, 2024

axw Jan 24, 2024

carsonip Jan 24, 2024

axw Jan 25, 2024

carsonip Jan 25, 2024

axw Jan 29, 2024

axw Jan 29, 2024

carsonip commented Jan 23, 2024

carsonip commented Jan 25, 2024

axw left a comment

Respect data_stream.{dataset,namespace} for OTel logs, metrics, and traces #201

Respect data_stream.{dataset,namespace} for OTel logs, metrics, and traces #201

Conversation

carsonip commented Jan 15, 2024 • edited Loading

Choose a reason for hiding this comment

carsonip Jan 16, 2024 • 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

joshdover commented Jan 16, 2024 • edited by felixbarny Loading

carsonip commented Jan 16, 2024

axw left a comment

Choose a reason for hiding this comment

carsonip commented Jan 22, 2024 • 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

carsonip commented Jan 23, 2024

carsonip commented Jan 25, 2024

axw left a comment

Choose a reason for hiding this comment

carsonip commented Jan 15, 2024 •

edited

Loading

carsonip Jan 16, 2024 •

edited

Loading

joshdover commented Jan 16, 2024 •

edited by felixbarny

Loading

carsonip commented Jan 22, 2024 •

edited

Loading