Skip to content
This repository has been archived by the owner on Feb 27, 2024. It is now read-only.

[WIP] Add parent span argument to SpanProcessor.OnStart #2

Closed
wants to merge 5 commits into from

Conversation

johananl
Copy link
Member

@johananl johananl commented Nov 6, 2020

No description provided.

@krnowak
Copy link
Member

krnowak commented Nov 6, 2020

I think this needs more discussion in the issue. Currently what we pass to the processor is a SpanData thing. But the spec is talking about passing a span and a parent span context. So I'm not sure here - span data already contains a parent span ID, so passing a parent span context seems redundant here. So either we change SpanData to contain parent span context instead of only parent span ID or we change it to pass a span and parent span context.

Other than that, the changes look ok, but they show that for now this parent span context is rather useless. I suppose that the batch span processor should actually be doing something with it…

The spec requires having this argument in OnStart.
According to the spec, OnStart and OnEnd should accept a span object
which is represented by the otel.Span interface in the Go library.
Right now we are passing a *export.SpanData struct to these methods
which is an SDK-internal struct.
When using the API we can't rely on attributes to determine the order
of span processors because SetAttributes overwrites attributes when a
for existing keys, which results in keeping only the attribute set by
the last span processor.

Before this change it was possible to use attributes to determine span
processor order because the tests wrote to the attributes slice
directly rather than via the SetAttributes API method.

Use events as a wrapper around attributes instead.
@johananl johananl force-pushed the johananl/onstart-parent-context branch from c9b800c to a8563f2 Compare November 12, 2020 16:09
@johananl johananl closed this Nov 12, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants