[enrichment/trace] Handle exception for span events #99

lahsivjar · 2024-09-23T18:25:05Z

Related to: #86

enrichments/trace/internal/elastic/span.go

lahsivjar · 2024-09-23T18:37:00Z

enrichments/trace/internal/elastic/span.go

+	// TODO(lahsivjar): Is error/log context handling be done in es-exporter?
+	// Ref: https://github.com/elastic/apm-data/blob/59d33b8113d629f43dc82f9b3931ceda63a19a7a/input/otlp/traces.go#L1243-L1260


[For reviewers] APM-data sets context for span-events where it sets fields like span.id, transaction.id, parent.id based on the span (ref), however, I am not sure if enrichments is the correct place to do this. Since the es-exporter could decide how to encode span-events into ES docs (separate or in the same doc as parent span) maybe it would be better to have this handled in the expoter? I am confused here, WDYT?

Yes, I think that all properties that are already mapped as top level fields in the logs data stream should not be handled by enrichments but in the ES exporter.

CC: @carsonip This would require some changes in the spanevent handling in es-exporter. I think just copying some attributes from the parent span should be good enough.

makes sense

span event span.id, transaction.id are already done in elasticsearchexporter https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/main/exporter/elasticsearchexporter/model.go#L645-L647

in that case, no change is needed?

Do we actually need transaction.type for span events?

I don't know

If so, should they be in attributes.* or at top level?

If needed, it should be under attributes.

Yes, I think that all properties that are already mapped as top level fields in the logs data stream should not be handled by enrichments but in the ES exporter.

If needed, it should be under attributes.

As they are under attributes, it is better done in enrichment

As they are under attributes, it is better done in enrichment

I don't think it is a good idea to do this as enrichments since the fields we are talking about here are derived from the span. The ES exporter decides HOW to extract and encode the span event (in a separate doc or in the same doc). Whlie currently we are encoding them as a separate doc making this dependent on enrichment doesn't feel correct to me.

(#99 (comment) discussion thread is also related to the same topic)

Me and @carsonip talked about this IRL and decided to do it as enrichments for now due to the differences in the handling of error and log contexts in apm-data (ref). Once the specs are a bit more clearer around this we can revisit the enrichment.

222386e

The above commit adds transaction.{type, sampled} enrichment for span event.

lahsivjar · 2024-09-23T18:38:39Z

enrichments/trace/internal/elastic/span.go

+			case semconv.AttributeExceptionEscaped:
+				s.exceptionEscaped = v.Bool()
+			case semconv.AttributeExceptionType:
+				s.exceptionType = v.Str()
+			case semconv.AttributeExceptionMessage:
+				s.exceptionMessage = v.Str()
+			case semconv.AttributeExceptionStacktrace:
+				s.exceptionStacktrace = v.Str()


@gregkalapos These attributes are mostly 1<>1 mappings however, I didn't see them in the otel-data plugin so added it here. Do you think we should do this here or in the otel-data plugin?

Maybe we can create alias fields for these?

Yeah, aliases are how we have been handling such fields. Will wait for @gregkalapos to confirm

Yes, I'd be also for aliases as well.

One exception (no pun intended) may be the handled flag: from what I remember from the UI testing, the UI access that field, so it always needs to be present (We could question why, but from what I remember, that's how it is today), but exception.escaped is only recommended. So we always need to populate the handled flag.

That's the only edge case which comes to my mind, I'd expect the other ones to work with aliases.

Sounds good. Could you add these aliases, Greg? One challenge will be how to add the attribute aliases just for logs, without these getting overridden with the standard set of attribute aliases. Maybe we'll need to add them to the alias definition that applies to all signals.
Also, I'm not sure if we need an alias for the exception stacktrace, since we likely need to fetch that from _source anyway, because IINM, we don't have a stored field for it?

But we don't even need the aliases, don't we? 🤔 This should all work out via passthrough.

Following could be sent via attributes and would be visible top level:

exception.type

exception.message
( above 2 are in SemConv - I don't think we even need to do anything with those here in the enrichment lib)

exception.handled: that is not part of OTel, se we need to set it here.

I opened a draft: elastic/elasticsearch#113636 I commented out all the aliases, the test is still green which queries exception.type top level.

@gregkalapos I think the alias should be for error.exception.{type, message} -> exception.{type, message}

Also, I think we might need to put alias for stacktrace too: error.stack_trace -> exception.stacktrace

Oh, I see.

Ok, we can do error.exception.{type, message} -> exception.{type, message}; I'll update the PR.

Same for stack trace.

felixbarny · 2024-09-24T07:48:53Z

enrichments/trace/internal/elastic/attributes.go

@@ -47,4 +47,15 @@ const (
 	AttributeSpanDestinationServiceResource = "span.destination.service.resource"
 	AttributeSpanDurationUs                 = "span.duration.us"
 	AttributeSpanRepresentativeCount        = "span.representative_count"
+
+	// span event attributes
+	AttributeParentID              = "parent.id"


I wonder if we should solve that by adding a parent.id -> span_id alias in the logs data stream.

I'm afraid the "parent" in parent.id is not the span, but the parent of span, i.e. parent.id != span_id.

I looked at the apm-data code, it appears that parent.id is really the span event's span's parent id. Not the span's id: https://github.com/elastic/apm-data/blob/cd0bb416903eebb5dc22e3eb6b6e4e9c75ad51fe/input/otlp/traces.go#L196

Parent id is passed onto the span event via cloning the span: https://github.com/elastic/apm-data/blob/cd0bb416903eebb5dc22e3eb6b6e4e9c75ad51fe/input/otlp/traces.go#L1072

I'm afraid the "parent" in parent.id is not the span, but the parent of span, i.e. parent.id != span_id.

I don't think this is true for errors. In case of errors, we specifically override the parent ID of the SpanEvent to be the SpanID (or `TransactionID for elastic transaction): https://github.com/elastic/apm-data/blob/bd5e34116889942c53b186deab0f76beaf182de0/input/otlp/traces.go#L1255-L1259

oh wow, I missed that. Then is it a bug for logs from span events in apm-server?

does this fix in apm-data look right? elastic/apm-data#374
If so, yes, I think we can have parent.id alias -> span.id.

we are discussing on slack the alternative to make the UI use span.id directly: https://elastic.slack.com/archives/C95SB62AG/p1727693736192519?thread_ts=1727462154.365409&cid=C95SB62AG

enrichments/trace/internal/elastic/attributes.go

felixbarny · 2024-09-24T07:51:09Z

enrichments/trace/internal/elastic/span.go

+	// TODO(lahsivjar): Is error/log context handling be done in es-exporter?
+	// Ref: https://github.com/elastic/apm-data/blob/59d33b8113d629f43dc82f9b3931ceda63a19a7a/input/otlp/traces.go#L1243-L1260


Yes, I think that all properties that are already mapped as top level fields in the logs data stream should not be handled by enrichments but in the ES exporter.

felixbarny · 2024-09-24T07:52:26Z

enrichments/trace/internal/elastic/span.go

+			case semconv.AttributeExceptionEscaped:
+				s.exceptionEscaped = v.Bool()
+			case semconv.AttributeExceptionType:
+				s.exceptionType = v.Str()
+			case semconv.AttributeExceptionMessage:
+				s.exceptionMessage = v.Str()
+			case semconv.AttributeExceptionStacktrace:
+				s.exceptionStacktrace = v.Str()


Maybe we can create alias fields for these?

enrichments/trace/internal/elastic/span.go

[enrichment/trace] Handle exception for span events

26a9fe2

lahsivjar requested a review from a team as a code owner September 23, 2024 18:25

lahsivjar commented Sep 23, 2024

View reviewed changes

lahsivjar requested review from axw, gregkalapos, felixbarny and carsonip September 23, 2024 18:39

Fix struct padding

3883a14

felixbarny reviewed Sep 24, 2024

View reviewed changes

lahsivjar mentioned this pull request Sep 24, 2024

[enrichment/trace] Use parsed stacktrace for calculating error grouping key #101

Open

lahsivjar added 2 commits September 27, 2024 13:00

Remove exception.{type,message} and stacktrace from enrichment

4361255

error/log context enrichment will be in es-exporter

80c485f

kruskall approved these changes Sep 27, 2024

View reviewed changes

lahsivjar added 2 commits September 27, 2024 18:30

Remove span.kind enrichment

82d649e

Add transaction.{sampled, type} as span event attribute

222386e

carsonip approved these changes Oct 1, 2024

View reviewed changes

felixbarny approved these changes Oct 1, 2024

View reviewed changes

lahsivjar merged commit 10e3f6c into elastic:main Oct 1, 2024
3 checks passed

lahsivjar deleted the spanevent branch October 1, 2024 14:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[enrichment/trace] Handle exception for span events #99

[enrichment/trace] Handle exception for span events #99

lahsivjar commented Sep 23, 2024 •

edited

Loading

lahsivjar Sep 23, 2024 •

edited

Loading

felixbarny Sep 24, 2024

lahsivjar Sep 24, 2024

carsonip Sep 24, 2024

carsonip Sep 24, 2024 •

edited

Loading

felixbarny Sep 27, 2024

carsonip Sep 27, 2024

lahsivjar Sep 30, 2024

lahsivjar Sep 30, 2024

lahsivjar Sep 30, 2024

lahsivjar Sep 23, 2024

felixbarny Sep 24, 2024

lahsivjar Sep 24, 2024

gregkalapos Sep 24, 2024

felixbarny Sep 25, 2024 •

edited

Loading

gregkalapos Sep 26, 2024

lahsivjar Sep 27, 2024

lahsivjar Sep 27, 2024

gregkalapos Sep 27, 2024

felixbarny Sep 24, 2024

carsonip Sep 26, 2024

lahsivjar Sep 27, 2024

carsonip Sep 27, 2024 •

edited

Loading

carsonip Sep 27, 2024

carsonip Sep 30, 2024

felixbarny Sep 24, 2024

felixbarny Sep 24, 2024

		// TODO(lahsivjar): Is error/log context handling be done in es-exporter?
		// Ref: https://github.com/elastic/apm-data/blob/59d33b8113d629f43dc82f9b3931ceda63a19a7a/input/otlp/traces.go#L1243-L1260

[enrichment/trace] Handle exception for span events #99

[enrichment/trace] Handle exception for span events #99

Conversation

lahsivjar commented Sep 23, 2024 • edited Loading

lahsivjar Sep 23, 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

carsonip Sep 24, 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

felixbarny Sep 25, 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

carsonip Sep 27, 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

lahsivjar commented Sep 23, 2024 •

edited

Loading

lahsivjar Sep 23, 2024 •

edited

Loading

carsonip Sep 24, 2024 •

edited

Loading

felixbarny Sep 25, 2024 •

edited

Loading

carsonip Sep 27, 2024 •

edited

Loading