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

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

Closed
lmolkova opened this issue Jun 21, 2024 · 3 comments · Fixed by #1241
Closed

Comments

@lmolkova
Copy link
Contributor

lmolkova commented Jun 21, 2024

  • There are no messaging systems that make any use of this namespace.
  • We don't know if there are any that could have the original destination available
  • We don't know if we'd ever have anything except the name attribute available to the consume
  • messaging.destination.original.name could be an alternative name.

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
Copy link
Contributor Author

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
Copy link
Contributor

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
Copy link
Contributor Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: V1 - Stable Semantics
3 participants