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

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

Show file tree
Hide file tree
Changes from 14 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
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ the release.
- Clarify Tracer vs TracerProvider in tracing API and SDK spec. Most importantly:
* Configuration should be stored not per Tracer but in the TracerProvider.
* Active spans are not per Tracer.
- Add peer.service to provide a user-configured name for a remote service

## v0.5.0 (06-02-2020)

Expand Down
7 changes: 5 additions & 2 deletions specification/trace/semantic_conventions/rpc.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,14 +65,17 @@ Furthermore, setting [net.transport][] is required for non-IP connection like na

#### Service name

On the server process receiving and handling the remote procedure call, the service name provided in `rpc.service` does not necessarily have to match the [`service.name` resource attribute][]. One process can expose multiple RPC endpoints and thus have multiple RPC service names. From a deployment perspective, as expressed by the `service.*` resource attributes, it will be treated as one deployed service with one `service.name`.
On the server process receiving and handling the remote procedure call, the service name provided in `rpc.service` does not necessarily have to match the [`service.name`][] or [`peer.service`][] resource attributes. One process can expose multiple RPC endpoints and thus have multiple RPC service names. From a deployment perspective, as expressed by the `service.*` resource attributes, it will be treated as one deployed service with one `service.name`.
anuraaga marked this conversation as resolved.
Show resolved Hide resolved

As an example, given a process deployed as `QuoteService`, this would be the name that goes into the `service.name` resource attribute which applies to the entire process.
This process could expose two RPC endpoints, one called `CurrencyQuotes` (= `rpc.service`) with a method called `getMeanRate` (= `rpc.method`) and the other endpoint called `StockQuotes` (= `rpc.service`) with two methods `getCurrentBid` and `getLastClose` (= `rpc.method`).

Generally, a user SHOULD not set `peer.service` to a fully qualified RPC service name as that will be redundant with `rpc.service`.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Generally, a user SHOULD not set `peer.service` to a fully qualified RPC service name as that will be redundant with `rpc.service`.
Generally, a user SHOULD not set `peer.service` if it would be equal to the RPC service name as that would be redundant with `rpc.service`.

Or do we want users to set peer.service to the unqualified name of the RPC 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.

Thanks or the suggestions - for this one, I don't think we should encourage anything and leave it to the user. Personally, I find an unqualified service name to be easier to use in monitoring than the fully qualified (fully qualified I use to ensure no code collisions in code, not since it looks nice in dashboards, it often doesn't).

Copy link
Member

Choose a reason for hiding this comment

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

I think there is yet another difference between the fully qualified RPC service name (that is used to connect to the service for example) and the fully qualified name of the (Java/Go/...) class that implements the service.

Copy link
Contributor Author

@anuraaga anuraaga Jun 29, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah - but I don't think people generally design their proto package names based on what they want to see in monitoring. In practice, I often see Java shops use a long FQDN proto package, C++ shops use a one-worder or so, and I've seen the package change from a one-worder to a FQDN because code stopped compiling due to a collision. Letting the user decouple this programming concept from monitoring seems nice for them. It's also nice that we have something to fallback to when they didn't though.

Copy link
Member

Choose a reason for hiding this comment

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

a) s/SHOULD not/SHOULD NOT/ (this is how RFC keywords work)
b) I am not sure about this sentence at all. If anything, I would rather people not send rpc.service, but do send peer.service, as the latter can be used in dependency diagrams, but rpc.service is meh (instrumentation at Uber just sets span.name={rpc.service}::{rpc.method} and does not send those extra tags at all, there's little use for them).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@arminru felt pretty strongly about adding this line. I'm ok with either though :)

Copy link
Member

Choose a reason for hiding this comment

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

I think the sentence emphasizes the wrong thing. I am not bothered by redundancy, because it is coincidental. Earlier we explain that service name is a reference to a deployment unit, like "k8s service". So I agree with the SHOULD NOT part, but because peer.service and rpc.service are completely different things. The paragraph above this one explains it quite well with the QuoteService example.

Ideally we should have this explanation in resource#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.

Thanks - didn't tweak resource.md, but I think I get what you're saying. I tweaked to remove the point about redundancy and tie into the above paragraph, hopefully this helps


[network attributes]: span-general.md#general-network-connection-attributes
[net.transport]: span-general.md#nettransport-attribute
[`service.name` resource attribute]: ../../resource/semantic_conventions/README.md#service
[`service.name`]: ../../resource/semantic_conventions/README.md#service
[`peer.service`]: span-general.md#general-remote-service-attributes

### Distinction from HTTP spans

Expand Down
13 changes: 13 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,19 @@ 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 operation that accesses some remote service. Users can define what the name of a service is based on their particular semantics in their distributed system.
anuraaga marked this conversation as resolved.
Show resolved Hide resolved
Instrumentation is expected to provide a way for users to configure this name.
anuraaga marked this conversation as resolved.
Show resolved Hide resolved

| Attribute name | Notes and examples |
| :-------------- | :-------------------------------------------------------------------------------- |
| `peer.service` | The (hypothetical) [`service.name`](../../resource/semantic_conventions/README.md#service) of the remote service. SHOULD be equal to the actual `service.name` resource attribute of the remote service if any. |
Copy link
Member

Choose a reason for hiding this comment

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

why "hypothetical"?

Copy link
Member

@Oberon00 Oberon00 Jun 29, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Because the other end might not be instrumented at all, e.g. see the Redis example below. The redis process will probably not be instrumented with OpenTelemetry.

Copy link
Member

Choose a reason for hiding this comment

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

That does not explain the term "hypothetical" - there's a real service on the other side, instrumented or not. If I am calling a service and, for example, use name X to discover that service, then peer.service=X and it's not hypothetical.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, I just meant the service.name resource attribute is hypothetical.

Copy link
Member

Choose a reason for hiding this comment

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

I would leave it simply as service.name without qualifiers, as the qualifier only makes the situation more confusing. If instrumentation doesn't know the peer service name, then the tag is optional.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think the "hypothetical" is confusing, especially if you also read the next sentence, but I'm also OK with removing it.

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 went ahead and removed hypothetical, don't think it's confusing, but I guess it's not needed either so can reduce the text a bit.


Examples of `peer.service` that users may specify:
- A Redis cache of auth tokens. `peer.service=AuthTokenCache`.
anuraaga marked this conversation as resolved.
Show resolved Hide resolved
- A gRPC service `rpc.service=io.opentelemetry.AuthService` may be hosted in both a gateway, `peer.service=ExternalApiService` and a backend, `peer.service=AuthService`.
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