open-telemetry / semantic-conventions

Defines standards for generating consistent, accessible telemetry across a variety of domains
Apache License 2.0
255 stars 165 forks source link

Should `messaging.destination_publish.*` be in the stabilization scope for messaging? #1178

Closed lmolkova closed 2 months ago

lmolkova commented 3 months ago

Given all of this, I'd like to exclude messaging.destination_publish.* from the stabilization scope or even deprecate it as something we don't know where/how to use.

lmolkova commented 3 months ago

Looking at existing usage of this attribute, it seems we've created some confusion with it:

  1. Both attributes are set, but somehow they are different for queues and topics?

    https://github.com/kalaspuff/tomodachi/blob/ef036831d59d23e18280af91288ad2c6046c26db/README.md?plain=1#L1393-L1394

    span.set_attribute("messaging.destination.name", queue_url.rsplit("/")[-1])
    span.set_attribute("messaging.destination_publish.name", topic or queue_url.rsplit("/")[-1])
  2. Different attributes are set depending on the kind https://github.com/airtai/faststream/blob/28f47833dd77509f4596fb582e6b539468c31fbc/tests/opentelemetry/basic.py#L102-L113

    if span.kind == SpanKind.PRODUCER and action in (Action.CREATE, Action.PUBLISH):
        assert attrs[SpanAttr.MESSAGING_DESTINATION_NAME] == queue, attrs[
            SpanAttr.MESSAGING_DESTINATION_NAME
        ]
    
    if span.kind == SpanKind.CONSUMER and action in (Action.CREATE, Action.PROCESS):
        assert attrs[MESSAGING_DESTINATION_PUBLISH_NAME] == queue, attrs[
            MESSAGING_DESTINATION_PUBLISH_NAME
        ]
        assert attrs[SpanAttr.MESSAGING_MESSAGE_ID] == IsUUID, attrs[
            SpanAttr.MESSAGING_MESSAGE_ID
        ]
  3. Destination_publish is set on producer for rabbitmq (to exchange) https://github.com/open-telemetry/opentelemetry-php-contrib/blob/9f96b59bc483cc495edc49cef0d63b637a46fffa/src/Instrumentation/ExtAmqp/src/ExtAmqpInstrumentation.php#L61

    ->setAttribute(TraceAttributes::MESSAGING_DESTINATION_NAME, $routingKey)
    ->setAttribute(TraceAttributes::MESSAGING_DESTINATION_PUBLISH_NAME, sprintf('%s%s', $exchange->getName() != '' ? $exchange->getName() . ' ': '', $routingKey))
    
pyohannes commented 3 months ago

Given all of this, I'd like to exclude messaging.destination_publish.* from the stabilization scope or even deprecate it as something we don't know where/how to use.

I second that.

One thing we should ensure for stabilization: that those attributes (or similar) attributes are not needed on metrics. They're not in the current list of attributes on metrics, however, I'd like us to validate that again. If there's no need for those attributes on metrics, let's exclude them from initial stability.

lmolkova commented 2 months ago

Discussed on messaging SIG 7/11: Let's deprecate messaging.destination_publish.* attributes since we have no real-life use-case for them and current usages are not correct.