-
Notifications
You must be signed in to change notification settings - Fork 821
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
Resources API: package, semantic conventions, and test utils #815
Conversation
6d2fad0
to
b10bcb7
Compare
Codecov Report
@@ Coverage Diff @@
## master #815 +/- ##
==========================================
- Coverage 94.55% 92.91% -1.65%
==========================================
Files 249 246 -3
Lines 10751 10868 +117
Branches 1060 1066 +6
==========================================
- Hits 10166 10098 -68
- Misses 585 770 +185
|
I believe that since it's only interface/enums, we should put that inside |
This is not just interfaces/enums. Also, I don't believe that resource is meant to be exposed by the API, but is a SDK concern. |
Co-Authored-By: Daniel Dyla <[email protected]>
I've taken this out of draft and into ready for review. I also updated the comment with a description of the current state of resources and the future work that we need to have clarified and make decisions on. I'm happy to take up any of that work in future PRs, or as part of this one if that's what we decide. |
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.
lgtm, just minor things
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.
👍
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.
Overall LGTM, added one minor nit.
"mocha": "^6.2.0", | ||
"nyc": "^14.1.1", | ||
"rimraf": "^3.0.0", | ||
"sinon": "^7.5.0", |
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.
Unused dep
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.
LGTM
Which problem is this PR solving?
Short description of the changes
Next
This PR only provides modeling of Resources, constants for the semantic conventions, and test utilities to validate a resource for conforming to the conventions.
According to the Resource spec spans and metrics should be associated with a resource: https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/sdk-resource.md#resource-sdk
At the Spec SIG call there was confusion over how this should be done and I created this issue for clarification: open-telemetry/opentelemetry-specification#491.
I suspect there will be additional work to associate telemetry with resources, but am proposing that we handle that in a subsequent PR as we get clarification. The approach that Ruby is taking is described in: open-telemetry/opentelemetry-specification#491 and we can go that route if we choose to.
I have also noticed that OpenCensus node has automatic resource detection, these are for system, cloud, container resources: open-telemetry/opentelemetry-specification#491. We should decide what logic we would like to port from Census. Again, I think we can handle these as separate PRs.