-
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
resource terminology and API #56
resource terminology and API #56
Conversation
When associated with the `SpanData` explicitly for out-of-band spans - | ||
`Resource` that is set on `Tracer` MUST be ignored. | ||
|
||
**TODO**: explain how resource is associated with metrics. |
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.
Let's add it later. I don't want to introduce an empty metrics-api.md
document in this PR.
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 few nits, otherwise LGTM.
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.
Let's merge this for the moment, I think I made a mistake in the Node definition by adding the ServiceInfo (pressure to support the service information because of the lack of clarification about resource).
Second, by merging two `Resource`s into a new one. The method [Merge](#merge) is | ||
used when a `Resource` is constructed from multiple sources - metadata API call | ||
for the host and environment variables for the container. So a single `Resource` | ||
will contain labels for both. |
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.
What is the reason for doing this on resource level (extending API surface with the Merge()
function) instead of doing the merge on the collection of labels?
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.
Merge
has a logic on how to deal with empty or duplicate values. In OC it was also doiung a merge of a field type
.
I agree it's not much to justify this API today. But it is what we have in Java reference API. Do you want to track it as a "API feedback issue"? Please file an issue
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.
Merge has a logic on how to deal with empty or duplicate values. In OC it was also doiung a merge of a field type.
That's just an artifact of having a merge in the first place, no? If it was up to the user to provide a map of labels, then they would have to deal with duplicates, not the spec. And the type would only be specified once.
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.
booked #62
* resource terminology and API * addressed code review comments
…h-releases Strengthen rules for GitHub Releases
Since this OTEP has been thoroughly discussed before entering the "proposed" stage, it should be ready for approval already.
Since this OTEP has been thoroughly discussed before entering the "proposed" stage, it should be ready for approval already.
* resource terminology and API * addressed code review comments
#36 and #47