-
Notifications
You must be signed in to change notification settings - Fork 895
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use a common attribute interface #717
Use a common attribute interface #717
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be best to have an attributes.md
in the spec directory since this is shared between trace and resources.
A resource is made by a collection of attributes. Resources MUST use the same | ||
`Attribute` definition used for [Span Attributes](../trace/api.md#set-attributes). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A resource is made by a collection of attributes. Resources MUST use the same | |
`Attribute` definition used for [Span Attributes](../trace/api.md#set-attributes). | |
A resource is a collection of [Attributes](../trace/api.md#set-attributes). | |
Resources MUST allow the same value types as Spans. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/"A resource is a collection of attributes"/"A resource is represented by a collection of attributes"/
of: string, int64, double, bool. | ||
- a collection of [attributes](../trace/api.md#set-attributes). | ||
|
||
Attribute values of `null` are considered to be used to create the resource and |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand this sentence.
Should we also reference the null handling of spans here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1. What is the motivation for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suspect this has to do with the pre-existing definition of Merge? null
is not a valid attribute value, so this seems unnecessary to state. (OTOH, I'd like it to be a valid attribute value, but it would still be unnecessary.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we link the definition of Attributes used by setAttributes
, I wanted to state that for resources, null
values should be handled slightly differently.
The SDK must support two ways to instantiate new resources. Those are: | ||
A resource is made by a collection of attributes. Resources MUST use the same | ||
`Attribute` definition used for [Span Attributes](../trace/api.md#set-attributes). | ||
The SDK MUST support two ways to instantiate new resources. Those are: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Those are:
You can't use semicolon to refer to sections. Instead, say "The SDK MUST support two ways to instantiate new resources: Create and Merge". It also makes the sentence self-contained, you don't need to read through to understand what those two ways are.
Alternative approach proposed here #722 |
@Oberon00 I did extract the attributes without seeing the comment, but I think it makes sense |
Closing in favor of #722 |
Fixes #446 conforming the Attribute definition also for Resources