-
Notifications
You must be signed in to change notification settings - Fork 40
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
Add Resource entity #240
Add Resource entity #240
Conversation
2942b77
to
c723f9b
Compare
|
||
/** An immutable collection of [[Attribute]]s. | ||
*/ | ||
final class Attributes private ( |
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.
Currently, most methods use varargs, e.g:
def resourceSpan[A](name: String, attributes: Attribute[_]*)
I wonder whether we should offer a new alternative too:
def resourceSpan[A](name: String, attributes: 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.
To be honest, I would completely replace Attribute with Attributes. Attribute looks like an unnecessary wrapper to me because, from my perspective, users are more interested in using a collection that provides interfaces for accessing particular values by keys or iterating over all of them. I can imagine that It might be useful as a smart constructor which validates passed values, but the collection can do the same. Maybe I don't see a use-case for it.
sdk/common/src/main/scala/org/typelevel/otel4s/sdk/Resource.scala
Outdated
Show resolved
Hide resolved
Excellent work! Looks good to me |
Ross suggests in #199 (reply in thread) that we should target this to |
I've added NoPublishPlugin to the |
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.
Sorry for the slow review cycle! Had a few more thoughts, but overall looks good!
sdk/common/src/test/scala/org/typelevel/otel4s/sdk/ResourceSuite.scala
Outdated
Show resolved
Hide resolved
sdk/common/src/test/scala/org/typelevel/otel4s/sdk/ResourceSuite.scala
Outdated
Show resolved
Hide resolved
sdk/common/src/main/scala/org/typelevel/otel4s/sdk/Resource.scala
Outdated
Show resolved
Hide resolved
sdk/common/src/main/scala/org/typelevel/otel4s/sdk/Resource.scala
Outdated
Show resolved
Hide resolved
sdk/common/src/main/scala/org/typelevel/otel4s/sdk/Attributes.scala
Outdated
Show resolved
Hide resolved
sdk/common/src/main/scala/org/typelevel/otel4s/sdk/Attributes.scala
Outdated
Show resolved
Hide resolved
sdk/common/src/main/scala/org/typelevel/otel4s/sdk/Attributes.scala
Outdated
Show resolved
Hide resolved
sdk/common/src/test/scala/org/typelevel/otel4s/sdk/AttributesProps.scala
Show resolved
Hide resolved
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.
Looks good, thanks!
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.
Thanks for iterating! Looks good, sorry I still had a few nitpicks but all very minor and non-blocking.
sdk/common/src/main/scala/org/typelevel/otel4s/sdk/Resource.scala
Outdated
Show resolved
Hide resolved
sdk/common/src/main/scala/org/typelevel/otel4s/sdk/Resource.scala
Outdated
Show resolved
Hide resolved
sdk/common/src/main/scala/org/typelevel/otel4s/sdk/Resource.scala
Outdated
Show resolved
Hide resolved
sdk/common/src/main/scala/org/typelevel/otel4s/sdk/Resource.scala
Outdated
Show resolved
Hide resolved
36efc82
to
de74548
Compare
It appears that we need to validate attribute values for resources, but I couldn't decide on the appropriate location to do so. It would be better to add a smart constructor for the attribute itself in the following pull requests, rather than adding validation for already created attributes within the resource's constructor.