Extend semantic conventions for RPC and allow non-gRPC calls

[arminru]

I generalized the semantic conventions to allow RPC systems other than gRPC.
Therefore, I added an attribute rpc.system (like we have for messaging systems as messaging.system here) to identify the RPC system being used and added a section for gRPC, defining rpc.system="grpc" in this case.
Once we use proper YAML definitions, we can add a list of well-known descriptors to be used in addition to the ones I mentioned as examples.

Additionally, I added the method name as an attribute so that it doesn't have to be parsed out of the span name. This also makes it available for sampling decisions since the span name is not reliable to use here (#468).

By requiring the net.transport attribute to be set for non-IP connections we ensure that connections via named pipe bindings, for example, can be properly detected.

I didn't touch the rpc.service attribute already present but was wondering if we should change it to be the fully-qualified service name including $package, rather than just the unqualified name. What do you think?

I'm also considering to add an attribute for the logical endpoint which might be different from the physical endpoint described using the net.peer.* attributes. This would be particularly significant in scenarios where a service registry is used to resolve endpoints. We can either add it here or discuss it separately on a follow-up PR.

Resolves #27.

[arminru]

@open-telemetry/specs-approvers Anyone willing to have a look? 🙂

[bogdandrutu]

Common RPC conventions?

[arminru]

Do you suggest to shorten the heading or are you questioning the wording?

[bogdandrutu]

Do we really need these conventions? Looks like duplicate information with the name. I would definitely not make them required.

[arminru]

rpc.system is not part of the span name
rpc.service was already there, I think it was taken over from OpenTracing where it was called peer.service
rpc.method is new

While the latter two are part of the suggested span name, I'd rather have them available without having to parse the name and decompose the components.
On all other semantic conventions the most valuable data, which is used to detect and distinguish services, is provided using attributes and not from parsing the span name, even though this results in some duplication.
I deem the span name less reliable since it's intended as a grouping key and/or to provide a human-readable friendly name rather than a definite, structured data format. I'd expect deviations from the span name guidelines defined in the spec more frequently than not adhering to the attributes defined therein. For example, one could add a "RPC call to xy" prefix to the span name above to "make it look nicer in the UI" even though this would not be allowed as per the spec.

[bogdandrutu]

Not sure I can buy the "send duplicate data" to avoid "parsing". How do we know which one is more expensive? I would simply not make them required.

[arminru]

Avoiding parsing is one of three reasons I stated in my comment above.
The main reason is that it's in my opinion the least reliable data point we have on a span. All other semantic conventions say something along "you should use this span name or alternatively that but you can also use something else if it suits better or is of lower cardinality", so I personally doubt that users/instrumenters will adhere 100% to the guidelines laid out regarding span names. As long as it still makes a good, low-cardinality grouping key, this would be mostly fine, however.
For detecting services from span, I certainly prefer well-defined unambiguous attributes over something that is the main identifier showing up in the UI. The human-readable span name in the UI might, IMHO, be more likely to be populated with something else, because it's more favorable, self-explaining, human-readable or otherwise preferred by users looking at the UI (even if the spec would be pretty strict about it in this particular semantic convention).

[bogdandrutu]

My fight is still that this should not be required. I am fine and would approve this change if that is removed.

[bogdandrutu]

I feel that required means will not be able to consume data without this field, which is not the case for this.

[arminru]

Alright, I'll make it recommended instead of required and we can revisit it if #653 brings any applicable changes to the definition of required.

[arminru]

@bogdandrutu I made rpc.method not required. Should I also remove the requirement for rpc.service that is already in place in the current spec version or leave it as it is?

[bogdandrutu]

Yes please rpc.service should be the same.

OffTopic: Probably we should have 3 options (we can discuss in an issue OPTIONAL, RECOMMENDED, REQUIRED) then we should have very few `REQUIRED.

[arminru]

@bogdandrutu done.

Your proposal sounds reasonable, we should discuss this on #653.

[anuraaga]

Had one thought about rpc.service. I think in tracing systems, having a good service name is very important for features like dependency graphs, and this applies to all spans. Does it make sense to move service.name to the span general attributes instead of having it RPC-only? I don't think I've seen a tracing system that doesn't handle service name in a special way as a first class citizen.

The example I am thinking of is Redis. There is no descriptive name for a redis database, it's up to how the user code uses it, so a redis may be named UserCache in block diagrams and as variable names but does not appear in the URL if it uses IP address or anywhere else. I don't think the DB semantic conventions can cover this, but it doesn't seem like it should be a RPC either. If Redis itself was traced, it's Resource would have the correct name, but for databases where we won't accept them to have spans (at least not for years I'd say), it's important that the client can set the service name and this seems like a generic concept across many types.

[Oberon00]

@anuraaga: @yurishkuro noted in #604 (comment) that they used a peer.service attribute in a way similar to what you propose. I think you should create an issue for that. It sounds useful, but not really related to rpc.

[anuraaga]

@Oberon00 thanks for the recommendation, filed #648 :)