Skip to content
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

Merged
merged 2 commits into from
Jun 3, 2019

Conversation

SergeyKanzhelev
Copy link
Member

#36 and #47

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.
Copy link
Member Author

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.

Copy link
Member

@songy23 songy23 left a 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.

specification/resources-api.md Show resolved Hide resolved
specification/resources-api.md Outdated Show resolved Hide resolved
specification/resources-api.md Outdated Show resolved Hide resolved
Copy link
Member

@bogdandrutu bogdandrutu left a 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).

terminology.md Show resolved Hide resolved
@SergeyKanzhelev SergeyKanzhelev merged commit 7e407f6 into open-telemetry:master Jun 3, 2019
@SergeyKanzhelev SergeyKanzhelev deleted the resourceAPI branch June 3, 2019 18:03
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.
Copy link
Member

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?

Copy link
Member Author

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

Copy link
Member

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

booked #62

SergeyKanzhelev added a commit to SergeyKanzhelev/opentelemetry-specification that referenced this pull request Feb 18, 2020
* resource terminology and API

* addressed code review comments
TuckTuckFloof pushed a commit to TuckTuckFloof/opentelemetry-specification that referenced this pull request Oct 15, 2020
rockb1017 pushed a commit to rockb1017/opentelemetry-specification that referenced this pull request Nov 18, 2021
…h-releases

Strengthen rules for GitHub Releases
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants