Skip to content
This repository has been archived by the owner on Apr 23, 2021. It is now read-only.

Expose SpanAttributes Keys #153

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Expose SpanAttributes Keys #153

wants to merge 1 commit into from

Conversation

fabianfett
Copy link

Motivation

For some tasks, it's good to have access to all keys in SpanAttributes.

Result

Happier instrument developers. 🤩🚀

@ktoso
Copy link
Collaborator

ktoso commented Oct 8, 2020

I've been thinking about this... are we really sure we need this?

Can you show the exact use case? The least API this has the better.
Would you not iterate over keys twice often if using keys?

@pokryfka
Copy link
Contributor

pokryfka commented Oct 8, 2020

Per open-telemetry sepcification https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/trace/api.md#span, which I personally agree with:

Spans are not meant to be used to propagate information within a process. To prevent misuse, implementations SHOULD NOT provide access to a Span's attributes besides its SpanContext.

@pokryfka
Copy link
Contributor

pokryfka commented Oct 8, 2020

Also, it has practical implications: "which" keys should be exposed.

Open-telemetry uses "." in attribute key names.

X-Ray annotation name should not have "." (does not apply to X-Ray metadata keys):

Keys must be alphanumeric in order to work with filters. Underscore is allowed. Other symbols and whitespace are not allowed.

Assuming there was API to get key names - should they be the original keys or mapped keys; should they contain only annotations or both annotations and metadata keys.

This is mapping issue related to X-Ray, other tracers and/or specs may have different constraints.

@ktoso
Copy link
Collaborator

ktoso commented Oct 8, 2020

Yeah, I'm aware of the wording in otel.

Note that presently we have forEach { key, value already, so the argument:

Also, it has practical implications: "which" keys should be exposed.

Would be about both; This PR is about adding a superficial keys func. We can definitely look again if that foreach should be removed as well; The foreach is kind of easy for implementing "foreach key which I understand and want to emit", or people doing raw "pass through everything" -- up to tracer impls what they want to do. Without a foreach an impl has to attempt to access all potential values that it might expect in there which does not make sense to me.

Open-telemetry uses "." in attribute key names.
X-Ray annotation name should not have "." (does not apply to X-Ray metadata keys):

I don't think this matters here at all -- attributes storage is opaque, whatever "you" put in there stays there, it will not perform any mapping.

If your tracer wants to understand "otel style" stored values, it may understand the "hello.world" formatted keys, otherwise it can just use it's own values.

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.

4 participants