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.

Sign up for GitHub

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

Jump to bottom

Add peer.service semantic convention to indicate the name of a target… #652

Merged
merged 23 commits into from
Jun 30, 2020
Merged

Add peer.service semantic convention to indicate the name of a target… #652

Changes from 5 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
23edfdb
Add peer.service semantic convention to indicate the name of a target…
Jun 12, 2020
2afb9c3
Merge branch 'master' of github.com:open-telemetry/opentelemetry-spec…
Jun 22, 2020
a3ee515
Apply suggestions from code review
anuraaga Jun 22, 2020
b096793
Merge branch 'peer-service' of github.com:anuraaga/opentelemetry-spec…
Jun 22, 2020
8478257
Simplify
Jun 22, 2020
e5deba1
Remove fallback wording
Jun 24, 2020
9390e6b
Needs to be configured using instrumentation
Jun 25, 2020
1a27724
Merge branch 'master' of github.com:open-telemetry/opentelemetry-spec…
Jun 26, 2020
045575a
CHANGELOG
Jun 26, 2020
2e42db3
Clarify relationship with rpc.service and peer.service and some examples
Jun 29, 2020
43520c0
Clarify example
Jun 29, 2020
9e28662
Move peer.service / rpc.service relationship explanation to rpc doc.
Jun 29, 2020
ca40c67
Apply suggestions from code review
anuraaga Jun 29, 2020
72601a9
Update specification/trace/semantic_conventions/span-general.md
anuraaga Jun 29, 2020
8a6b429
Cleanup
Jun 30, 2020
f38b6f3
Tweak
Jun 30, 2020
8c00387
Merge branch 'master' of github.com:open-telemetry/opentelemetry-spec…
Jun 30, 2020
3ee9b1c
Update specification/trace/semantic_conventions/rpc.md
anuraaga Jun 30, 2020
f09a7f3
Update CHANGELOG.md
anuraaga Jun 30, 2020
7d61b00
Apply suggestions from code review
anuraaga Jun 30, 2020
9bdffa9
Update specification/trace/semantic_conventions/span-general.md
anuraaga Jun 30, 2020
55e2799
TOC
Jun 30, 2020
d3be175
Apply suggestions from code review
anuraaga Jun 30, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions specification/trace/semantic_conventions/span-general.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,15 @@ If `net.transport` is `"unix"` or `"pipe"`, the absolute path to the file repres
If there is no such file (e.g., anonymous pipe),
the name should explicitly be set to the empty string to distinguish it from the case where the name is just unknown or not covered by the instrumentation.

## General remote service attributes
arminru marked this conversation as resolved.
Show resolved Hide resolved

These attributes may be used for any remote operation that applies to a service. Users will generally define what a service is, though instrumentation
Copy link
Member

Choose a reason for hiding this comment

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

Please clarify what should be done if a closer matching specific semantic convention already defines a dedicated attribute like the RPC semantic conventions with their rpc.service attribute.

a) Should only the dedicated, specific attribute be set?
b) Should both be set to the same value? (probably not)
c) Does peer.service have a different semantic? If so, which?
d) Should rpc.service (and others, if any) be removed and instead reference/require peer.service?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm I'm thinking I should remove the point about automatically falling back to anything. I consider this a knob for users to customize how their traces look since in the end they'll often know best. One example is where the same proto file is mounted in a proxy and backend, then this really needs to be set by a user such that they can be differentiated.

On the flip side, I can't think of a good reason to fallback anything automatic anymore.

Will your point be cleared up if I remove the point about fallback?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For now, removed the point about fallback, let me know if this could still use more detail

Copy link
Member

Choose a reason for hiding this comment

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

@anuraaga I was not concerned about the fallback wording but about the collision with existing, more specific attributes (at least rpc.service). This should be clarified. Which of the options I mentioned above a)-d) sounds most reasonable to you?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd lean towards b) since this is basically a spot for users to customize - if they customized to be the same as the rpc service, we may as well preserve that as they probably have a reason for it. It's why I thought the fallback wording could be related - we could have some dedupe semantics when doing something automatic, but for this field since it's for users, I think we could give them free reign. Let me know if that makes sense, and if I can explain that better. I'm balancing vs not wordsmithing too much or would be happy to add some examples. @yurishkuro any thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

If it would be the same value, it should be fine to just not set peer.service in this case as it doesn't add any value and it's optional anyway. If users have a named service that is one hierarchy level above the RPC endpoint's service identifier, setting different values could make sense. I could, for example, think of a ShopBackend service (= peer.service) that exposes a Pricing RPC endpoint (= rpc.service) with a getPrice method (= rpc.method) among other RPC services. It would then be up to the backend, however, to put them in the right hierarchy or if that's not supported prefer one of the service names over the other.
Since this is not obvious, please mention that both service names could be the same (if the RPC endpoint is directly exposed, suggesting to leave the peer.service out) or different (if there is a logical hierarchy encompassing one or multiple RPC endpoints, thus both attributes should be specified) either here or in the RPC semconv.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I went ahead and added a note to rpc.md :)

may provide a fallback when not user-controlled based on instrumentation-specific behavior.

| Attribute name | Notes and examples |
| :-------------- | :-------------------------------------------------------------------------------- |
| `peer.service` | Logical name of the service. <br/> MUST be the same for all instances of horizontally scaled services. |
anuraaga marked this conversation as resolved.
Show resolved Hide resolved

## General identity attributes

These attributes may be used for any operation with an authenticated and/or authorized enduser.
Expand Down