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 support for Pika #217

Merged
merged 1 commit into from
Jun 25, 2024
Merged

Add support for Pika #217

merged 1 commit into from
Jun 25, 2024

Conversation

luismiramirez
Copy link
Member

Add instrumentation support for Pika messages. The global messaging extractor already covers the cases this instrumentation adds, so there's no need for a specific extractor so far.

Action with message production:

image

Consumption action automatically instrumented

image

Part of: #181

@luismiramirez
Copy link
Member Author

Test setup: appsignal/test-setups#212

Add instrumentation support for Pika messages. The global messaging
extractor already covers the cases this instrumentation adds, so there's
no need for a specific extractor so far.
@luismiramirez
Copy link
Member Author

@unflxw
Copy link
Contributor

unflxw commented Jun 18, 2024

TL;DR: The instrumentation does some wacky stuff here that our global extractor is not ready to handle. I think it would be good to add a custom extractor for Pika in the agent to fix this.


[This is some explanation about the weird stuff the global extractor is doing with these spans and why -- feel free to skip it]

There are two spans here, one for sending the span and another for receiving it, that the Pika instrumentation for Python, in the test setup, sends with the names "test send" and "test receive".

These are actually pretty great span names! They are low cardinality, and they refer to the something that is meaningful to the user (the queue name, in this case "test") and a clear action ("send", "receive")

But then, because the attributes are not correctly set, the global extractor does not know what to do with these spans, and it messes them up in different ways.

For the send span, because there is no messaging.operation value (it should be "publish") the global extractor mostly doesn't know what to do with it. It does not set a good category (something like "send.rabbitmq" or "publish.rabbitmq", instead of "unknown.rabbitmq") and it does not change the name (this is actually good)

For the receive span, the opposite happens. Because there is a messaging.operation value ("receive") the global extractor sets the category (good) and rewrites the name.

But, when it rewrites the name, because messaging.destination is set to the customer tag (an opaque, high-cardinality value) instead of to the queue name ("test"), it messes it up, and rewrites it as something like "receive message (ctag.68b329da9893e34099c7d8ad5cb9c940)", which is less useful for the user than the queue name. Also, it's high-cardinality, which messes with our servers.


Thankfully this is why we have extractors! I would suggest a very simple extractor for Pika that doesn't use the messaging global extractor at all, instead just leaving the span name as-is, and setting the category using the last word of the span name (so, "send.pika", or "receive.pika"). Does that sound reasonable?

@unflxw
Copy link
Contributor

unflxw commented Jun 18, 2024

Ah, the agent is also merging the trace for the consumer and the producer (because they are propagated as the same trace)

Ideally, these would be two separate traces, like in Sidekiq, Celery or BullMQ. But this is harder to fix, so maybe we can leave it like this for now.

@luismiramirez
Copy link
Member Author

thanks for the insight @unflxw, I'll work on an extractor

@backlog-helper

This comment has been minimized.

2 similar comments
@backlog-helper

This comment has been minimized.

@backlog-helper
Copy link


This is a message from the daily scheduled checks.

New issue guide | Backlog management | Rules | Feedback

@luismiramirez luismiramirez merged commit 1fa8781 into main Jun 25, 2024
9 checks passed
@luismiramirez luismiramirez deleted the add-pika-support branch June 25, 2024 08:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants