Add Resource SDK #101
Add Resource SDK #101
Conversation
Codecov Report
@@ Coverage Diff @@
## master #101 +/- ##
============================================
+ Coverage 90.38% 91.00% +0.61%
- Complexity 53 57 +4
============================================
Files 15 16 +1
Lines 364 389 +25
============================================
+ Hits 329 354 +25
Misses 35 35
Continue to review full report at Codecov.
|
|
Lalex, I think this is a very great start on the Resource SDK and TracerCreation Library! You've done a great job of being conscientious of impacting changes; thank you for doing that. I like your recommendation of We should probably create a ticket to make sure that the Attributes collection isn't mutable if that's required to keep the Resource as an immutable object. The spec doesn't explicitly mention that the Attributes collection should be immutable, so maybe we should ask the spec maintainers if that was intentional. |
sdk/Resource/ResourceConstants.php
Outdated
| /** | ||
| * Service | ||
| */ | ||
| public const SERVICE_NAME = 'service.name'; |
There was a problem hiding this comment.
Should we somehow denote whether or not these are required? Might be as simple as a comment after the ; - Just thinking for when we are trying to grok them in the future.
| /** | ||
| * Kubernetes | ||
| */ | ||
| public const K8S_CLUSTER_NAME = 'k8s.cluster.name'; |
There was a problem hiding this comment.
Surprising that Github highlights these differently than the other public const variables. My editor did not do the same thing.
sdk/Resource/ResourceInfo.php
Outdated
|
|
||
| public function getAttributes(): Attributes | ||
| { | ||
| // todo: Attributes::setAttribute() breaks immutability of the Resource |
There was a problem hiding this comment.
Are you planning on breaking this todo out into a separate PR, or are you planning on including it as part of this draft?
sdk/Trace/TracerProvider.php
Outdated
| @@ -43,6 +49,8 @@ class TracerProvider implements API\TracerProvider | |||
| } | |||
|
|
|||
| $this->spanProcessors = $spanProcessors; | |||
| // todo: get Resource from arguments | |||
There was a problem hiding this comment.
Are you planning on breaking these todos out into a separate PR, or are you planning on including it as part of this draft?
| * A Resource is an immutable representation of the entity producing telemetry. For example, a process producing telemetry | ||
| * that is running in a container on Kubernetes has a Pod name, it is in a namespace and possibly is part of a Deployment | ||
| * which also has a name. All three of these attributes can be included in the Resource. | ||
| */ |
There was a problem hiding this comment.
It might be prudent to make a note about how we are using ResourceInfo rather than resource here because of the reserved PHP word.
|
@lalex do you have additional thoughts here? |
|
|
|
@bobstrecansky I've merged the latest master and resolved todos and notes. |
|
Verifying that no one else has any qualms in the gitter channel; will merge afterwords. |
This is an implementation of
ResourceSDK (#92) and recently addedInstrumentationLibrary(TracerCreation).In this PR minimal changes were made to
TracerProviderandTracerto simplify a merge with the concurrent PRs.The class for
Resourceis calledResourceInfobecauseresourceis the reserved word in PHP and it makes difficulties for code analysis tools. Any suggestion on name are welcome.In specification
Resourceis proposed as immutable object. Now it is not fully immutable since it usesAttributescollection which is mutable.