Span should have a getter for attributes

[codeboten]

Currently there are multiple places in the code that make the assumption that a Span has an attributes attribute, this is not always the case.

The solution could be to implement an attributes property on the Span interface, or we could implement a separate getter method.

[aravinsiva]

I can take this on if help is still needed.

[aravinsiva]

For this issue do all instances of attributes.get() need to be refractored to the new getter method (i.e. span.get_attribute(key)?

[carlosalberto]

The solution could be to implement an attributes property on the Span interface, or we could implement a separate getter method.

Would this be in the main OTel API? Asking cause we don't have that one in the other languages (they do exist in the SDK though).

[aravinsiva]

No right now just implementing in the sdk but I can add it ton the API if need be

[codeboten]

Tthe main problem that was being captured by this issue is that many instrumentations are calling out the a span's attributes directly. This is fine for access the SDK's implementation of a Span but that's not the case for the api, which has no attributes variable. There's a definition for a setter but not for a getter in the API for attributes, and maybe there should be.

[toumorokoshi]

Tthe main problem that was being captured by this issue is that many instrumentations are calling out the a span's attributes directly. This is fine for access the SDK's implementation of a Span but that's not the case for the api, which has no attributes variable. There's a definition for a setter but not for a getter in the API for attributes, and maybe there should be.

It doesn't necessarily needed to be added to the API, but it does requires the need for an SDK-specific noop span that returns nothing for the getter.

That said I support more explicitly providing a "Span" object in the API, and having an attribute getter with that.

[aabmass]

@aravinsiva, I think you discussed this (or was it something different?) in the Spec SIG last week. Did you come to any conclusion if this should be added to the Spec API?

[aravinsiva]

It was something different. Last week in the spec SIG I was talking about default span attributes in OT-js. If you feel like this is something that should be brought up in the next spec SIG I would be happy to do that.

[lzchen]

Does the existence of attributes in span creation imply that whatever implementation of Span is used, attributes should be an attribute? How is it possible to enforce attributes within an interface (I don't think you can?). If not, I think we'll need to use getter methods in the API, in case the user is the default span implementation.

Also, wouldn't this problem occur for all api level properties of the span? (events, links, spankind)

[aravinsiva]

After speaking to the specification SIG it seems as though the API was designed to be write only it and this is intentional. As a result we may need to develop another approach to solving the this issue. As @lzchen mentioned this is something that also occurs with events, links and spankind attributes. Since this does not seem to be getting raised to the spec level we may need to detemermined another approach to solving this issue. @codeboten suggested maybe looking for all instances of the attributes value being accessed and seeing if it exists. This is however a likely bad solution which won't enforce this down the line. As a result is there any solution to this issue as of right now? If so I would love to hear your guy's thoughts.

[lzchen]

That said I support more explicitly providing a "Span" object in the API, and having an attribute getter with that.

I think this would be the easiest, least intrusive solution. Even though "get" is not defined in the API specs, the issue seems language specific, and some other languages don't seem to be using a no-op span.

[aravinsiva]

Please reassign bug as internship is concluding

[toumorokoshi]

I think there's a way to resolve this without changing the spec. Namely:

• extending the No-op span in the SDK with getters allowed in the spec. Returning trace.sdk.NoopSpan where tracer.NoopSpan is used today.
• extending the other spans in the sdk from the no-op span.

I can take the issue, if there's consensus on the approach. @codeboten?

[lzchen]

extending the No-op span in the SDK

Isn't the no-op span in the api?

Returning trace.sdk.NoopSpan where tracer.NoopSpan is used today.

I'm not sure I understand the reasoning for this. Are you planning to move the no-op span implementation to the sdk?

@toumorokoshi

[toumorokoshi]

@lzchen the intention is to extend the no-op span in the sdk. So we'd have:

• trace.NoopSpan
• trace.sdk.NoopSpan

The main difference is the latter has getters. The reasoning is because getters aren't supposed to be in the API, but are supposed to be in the SDK.

[lzchen]

@toumorokoshi

Would this address the original issue of an attribute error though? Replacing tracer.NoopSpan with trace.sdk.NoopSpan would only cover the cases where we use the defaultspan in the sdk (like if the span is not sampled). But for cases like the instrumentations, in which we only take a dependency on the api, users do not have to use trace.sdk...., in which we would still run into the problem of accessing span attributes that possibly might not exist.

[toumorokoshi]

@lzchen I just did a scan through the code. No instrumentation is dependent on the .attributes method. The value is used in:

• exporters, which are designed to work with the SDK
• unit tests, which install the SDK to have a practical codebase to test against

What instrumentations depends on retrieving attributes? I'd argue that's an anti-pattern, as the API explicitly states that shouldn't be provided, and instrumentations should only work with the API methods.

[lzchen]

@lzchen I just did a scan through the code. No instrumentation is dependent on the .attributes method. The value is used in:

Yes I was just using an example. But I believe what you are saying is true about it being an anti-pattern, they should be using the api methods provided, so whoever creates them should be mindful of that.

I am fine with the proposed idea.

[shovnik]

@toumorokoshi I just wanted to confirm whether it has finally been decided to add a getter in sdk to access span attributes as it relates to the issue I am looking into mentioned above by lzchen. I am not sure whether to make the attributes harder to access or simply rely on the docs.

[toumorokoshi]

@shovnik getters in the SDK should be provided, as it's part of the spec.

Can you clarify on the work you're doing, and how that affects accessibility of attributes?

[shovnik]

@toumorokoshi

I was just trying to decide the best way to make the attributes inaccessible or if I should rely on docs. Since there currently wasn't a getter, I figured making the attributes private in some way right now would make them completely inaccessible. As suggested, I will attend this week's SIG to see if there is an agreed upon standard approach for controlling access rather than treating each future case individually.

[toumorokoshi]

@toumorokoshi

I was just trying to decide the best way to make the attributes inaccessible or if I should rely on docs. Since there currently wasn't a getter, I figured making the attributes private in some way right now would make them completely inaccessible. As suggested, I will attend this week's SIG to see if there is an agreed upon standard approach for controlling access rather than treating each future case individually.

What is the motivation for making the attributes private? Again according the spec "attributes" should be accessible in the SDK.

[codeboten]

Instrumentation should never access the attributes directly, the SDK having access to attributes is fine. Closing this issue