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

Confirm using destination as span name for messaging #686

Closed
anuraaga opened this issue Jul 2, 2020 · 8 comments · Fixed by #690
Closed

Confirm using destination as span name for messaging #686

anuraaga opened this issue Jul 2, 2020 · 8 comments · Fixed by #690
Assignees
Labels
area:semantic-conventions Related to semantic conventions release:required-for-ga Must be resolved before GA release, or nice to have before GA spec:trace Related to the specification/trace directory

Comments

@anuraaga
Copy link
Contributor

anuraaga commented Jul 2, 2020

I noticed that the span name convention for messaging seems different from any of our others. It is a noun, the target of an operation.

The span name should usually be set to the message destination name.

All of our other span names are the name of the operation, such as a method, I think.

  • The span name SHOULD be set to a low cardinality value representing the statement executed on the database.
  • Span name should be set to the function name being executed.
  • Therefore, HTTP client spans SHOULD be using conservative, low cardinality names formed from the available parameters of an HTTP request, such as "HTTP {METHOD_NAME}".
  • The span name MUST be the full RPC method name formatted as:

The general guidelines for span names don't seem to provide a push towards operation name or not, but just wanted to confirm that these shouldn't be named e.g., send, receive? I imagine a trace like this with DB and messaging mixed together as tuples of service name + span name

(MyService, MyMethod)
  (AuthCache, Get)
  (Kafka, OrdersTopic)
  (AuthCache, Set)

where one span name seems off compared to if the span name was the operation

(MyService, MyMethod)
  (AuthCache, Get)
  (OrdersTopic, Send)
  (AuthCache, Set)
@Oberon00
Copy link
Member

Oberon00 commented Jul 2, 2020

The span name SHOULD be set to a low cardinality value representing the statement executed on the database.

The statement includes any tables "targeted".

Therefore, HTTP client spans SHOULD be using conservative, low cardinality names formed from the available parameters of an HTTP request, such as "HTTP {METHOD_NAME}".

This applies only to clients, the server will usually use the route (URL template).

The fallback span name for HTTP clients is really not a good name but the best we could come up with, given the potential high cardinality of the host name.

I think messaging is more similar to dbs. We could include the operation in the span name (PUT/RETRIEVE/PEEK/PROCESS {destination}), but using only the operation would IMHO lead to too generic span names (you would only have one or two different messaging span names per app).

@Oberon00 Oberon00 added the spec:trace Related to the specification/trace directory label Jul 2, 2020
@arminru
Copy link
Member

arminru commented Jul 2, 2020

In general, if only the span name is used as a grouping key on the backend, names like "HTTP GET", that just group all GET requests together regardless of the target, don't make much sense to me and neither do they make a helpful name in the UI, so I think this would be something that could be reconsidered.

As for messaging, I think the queue name is most reasonable for both grouping and as a UI identifier but it could be enriched with the operation (send/receive) although the backend could take care of taking the span kind (producer/consumer and client/server) into account for both grouping and UI.

In your example, you're using the topic name OrdersTopic as the service name although the correct service name used for (span.)peer.service on both client and server spans would be the Queueing service, e.g. ShopKafka, and the topic would be missing in your tuple used for grouping and UI if only the operation was set as span name.

@Oberon00
Copy link
Member

Oberon00 commented Jul 2, 2020

Relatedly, guidelines for the span name might be clarified/changed in response to the discussion at #557 "Span name: Both low-cardinality (grouping key) and human-readable (display name)"

@reyang reyang added the release:required-for-ga Must be resolved before GA release, or nice to have before GA label Jul 6, 2020
@anuraaga
Copy link
Contributor Author

anuraaga commented Jul 7, 2020

Sorry for getting back to this late - yeah I see the point that for grouping, e.g., a drop down, including the topic is useful. I've always thought of span name as something that's a fallback - for backends that haven't really caught up with opentelemetry or using a shim, we can expect span name to be the universal identifier that all tracing systems support, so prioritizing human readability seemed to make sense. But it looks like #557 is proposing a separate field which also seems fine.

That being said, for messaging, since we know the cardinality of the operation is extremely low, send, receive, process. Would it still make sense to prefix the topic with these in the span name to give more information in the UI without sacrificing the grouping aspect in basic backends? Sophisticated backends can use the rich semantic attributes instead, IIUC this is similar to how we have service + method in the span name and as attributes for RPC.

In your example, you're using the topic name OrdersTopic as the service name although the correct service name used for (span.)peer.service on both client and server spans would be the Queueing service, e.g. ShopKafka

There's no correct name for service name IMO. It's really up to users what they want to see in their dashboards. I have migrated a service from RPC to messaging before and didn't see a need to change the service name, that aspect was an implementation detail, so I definitely wouldn't expect Kafka in there. But some users probably would do it it's up to them :)

@Oberon00
Copy link
Member

Oberon00 commented Jul 7, 2020

Would it still make sense to prefix the topic with these

IMHO, yes 👍 But we would need a proper list of possible operation names in the spec then.

@anuraaga
Copy link
Contributor Author

anuraaga commented Jul 7, 2020

Yup, it seems we have it already

https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/trace/semantic_conventions/messaging.md#messaging-attributes

messaging.operation defines only three possible values

@Oberon00
Copy link
Member

Oberon00 commented Jul 7, 2020

Hmm, it says "If the operation is send, this attribute must not be set", but by doing that I guess it does implicitly define a "send" operation.

@arminru
Copy link
Member

arminru commented Jul 7, 2020

@anuraaga For backends that use the span name as the main UI identifier it would probably make sense to have the topic name first and the verb as a suffix, so that sorted lists show span (groups) sorted per topic/queue and don't list all processing, receiving and then sending operations. I'll open a PR updating the span name guidance.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:semantic-conventions Related to semantic conventions release:required-for-ga Must be resolved before GA release, or nice to have before GA spec:trace Related to the specification/trace directory
Projects
None yet
4 participants