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

Add yaml semantic conventions for RPC #925

Merged
merged 2 commits into from
Sep 24, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
35 changes: 35 additions & 0 deletions semantic_conventions/trace/rpc.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
groups:
- id: rpc
prefix: rpc
brief: 'This document defines semantic conventions for remote procedure calls.'
attributes:
- id: system
type: string
required: always
brief: 'A string identifying the remoting system.'
examples: ["grpc", "java_rmi", "wcf"]
- id: service
type: string
required:
conditional: "No, but recommended"
brief: 'The full name of the service being called, including its package name, if applicable.'
examples: "myservice.EchoService"
- id: method
type: string
required:
conditional: "No, but recommended"
brief: 'The name of the method being called, must be equal to the $method part in the span name.'
examples: "exampleMethod"
- ref: net.peer.ip
- ref: net.peer.name
- ref: net.peer.port
required:
conditional: "See below"
- ref: net.transport
required:
conditional: "See below"
constraints:
- any_of:
- net.peer.ip
- net.peer.name
- include: network
28 changes: 17 additions & 11 deletions specification/trace/semantic_conventions/rpc.md
Original file line number Diff line number Diff line change
Expand Up @@ -48,17 +48,23 @@ Examples of span names:

### Attributes

| Attribute name | Notes and examples | Required? |
| -------------- | ---------------------------------------------------------------------- | --------- |
| `rpc.system` | A string identifying the remoting system, e.g., `"grpc"`, `"java_rmi"` or `"wcf"`. | Yes |
| `rpc.service` | The full name of the service being called, including its package name, if applicable. | No, but recommended |
| `rpc.method` | The name of the method being called, must be equal to the $method part in the span name. | No, but recommended |
| `net.peer.ip` | See [network attributes][]. | See below |
| `net.peer.name` | See [network attributes][]. | See below |
| `net.peer.port` | See [network attributes][]. | See below |
| `net.transport` | See [network attributes][]. | See below |

At least one of [network attributes][] `net.peer.name` or `net.peer.ip` is required.
<!-- semconv rpc -->
| Attribute | Type | Description | Example | Required |
|---|---|---|---|---|
| `rpc.system` | string | A string identifying the remoting system. | `grpc`<br>`java_rmi`<br>`wcf` | Yes |
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 examples should be quoted. Also I don't know if we want to use <br> by default. In this case , would be enough. So I think ideally this should look like

Suggested change
| `rpc.system` | string | A string identifying the remoting system. | `grpc`<br>`java_rmi`<br>`wcf` | Yes |
| `rpc.system` | string | A string identifying the remoting system. | `"grpc"`, `"java_rmi"`, `"wcf"` | Yes |

but just adding the quotes would also be good.

| `rpc.service` | string | The full name of the service being called, including its package name, if applicable. | `myservice.EchoService` | Conditional<br>No, but recommended |
| `rpc.method` | string | The name of the method being called, must be equal to the $method part in the span name. | `exampleMethod` | Conditional<br>No, but recommended |
Copy link
Member

@Oberon00 Oberon00 Sep 7, 2020

Choose a reason for hiding this comment

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

"Conditional. No" So it's not conditionally required? That sounds a bit odd. Maybe the tool should just not emit "conditional" but just put the note in? Or there should be an additional level between not required and required named "recommended". Or this should be just not required.

Copy link
Member Author

Choose a reason for hiding this comment

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

I also noticed this problem. Imho the tool should be updated to only emit the specified text without the Conditional prefix.

Copy link
Member

Choose a reason for hiding this comment

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

That sounds also fine, although it might lead to mis-use. Like "Required: Yes, because bla"

Copy link
Member

Choose a reason for hiding this comment

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

+1 to what @Oberon00 said. I am not sure how to read the "Conditional No, but recommended" phrase.

Copy link
Member

Choose a reason for hiding this comment

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

I filed open-telemetry/build-tools#7 for having this fixed in the MD generator. @thisthat will work on this soon so I would be fine with merging this PR and re-generate the updated markdown files in a follow-up.

| [`net.peer.ip`](span-general.md) | string | Remote address of the peer (dotted decimal for IPv4 or [RFC5952](https://tools.ietf.org/html/rfc5952) for IPv6) | `127.0.0.1` | See below |
| [`net.peer.name`](span-general.md) | string | Remote hostname or similar, see note below. | `example.com` | See below |
| [`net.peer.port`](span-general.md) | number | Remote port number. | `80`<br>`8080`<br>`443` | Conditional<br>See below |
| [`net.transport`](span-general.md) | string enum | Transport protocol used. See note below. | `IP.TCP` | Conditional<br>See below |

**Additional attribute requirements:** At least one of the following sets of attributes is required:

* [`net.peer.ip`](span-general.md)
* [`net.peer.name`](span-general.md)
<!-- endsemconv -->

For client-side spans `net.peer.port` is required if the connection is IP-based and the port is available (it describes the server port they are connecting to).
For server-side spans `net.peer.port` is optional (it describes the port the client is connecting from).
Furthermore, setting [net.transport][] is required for non-IP connection like named pipe bindings.
Expand Down