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

[Symfony auto-instrumentation] Messenger instrumentation should use SpanKind::KIND_PRODUCER, SpanKind::KIND_CONSUMER #1314

Open
dkarlovi opened this issue May 20, 2024 · 17 comments · Fixed by open-telemetry/opentelemetry-php-contrib#304
Assignees
Labels
bug Something isn't working good first issue Good for newcomers help wanted This issue is looking for someone to work on it

Comments

@dkarlovi
Copy link
Contributor

dkarlovi commented May 20, 2024

Steps to reproduce

Enable Symfony auto-instrumentation with default setup.

What is the expected behavior?

Spans when creating the message should be marked with SpanKind::KIND_PRODUCER.

Spans when handling the message should be marked with SpanKind::KIND_CONSUMER.

https://opentelemetry.io/docs/specs/semconv/messaging/messaging-spans/#span-kind

What is the actual behavior?

Everything is marked KIND_INTERNAL and it seems the consumer is currently not instrumented at all (I might be wrong here, please feel free to correct me).

Additionally, it seems to me the names of the spans are also not correct, as shown in the table in the spec linked above?

@dkarlovi dkarlovi added the bug Something isn't working label May 20, 2024
@agoallikmaa agoallikmaa added help wanted This issue is looking for someone to work on it good first issue Good for newcomers labels May 29, 2024
@AA2512
Copy link

AA2512 commented Aug 22, 2024

@agoallikmaa @brettmc
Can you please assign this to me

@RichardChukwu
Copy link

Hello @AA2512 please I'd like to know if this is still being worked please? @brettmc for possible assigning

Thank you

@brettmc
Copy link
Collaborator

brettmc commented Oct 6, 2024

Hello and welcome, @RichardChukwu
Please feel free to submit to work on this and submit a PR.

@RichardChukwu
Copy link

Got it, I'll do just that. Thank you @brettmc

@chiomalovet
Copy link

Hi @brettmc can i take this up

@dkarlovi
Copy link
Contributor Author

dkarlovi commented Oct 8, 2024

@chiomalovet it was just assigned to @RichardChukwu, unless he changed his mind, he should get dibs for a while to give it a shot.

@chiomalovet
Copy link

chiomalovet commented Oct 8, 2024 via email

@dkarlovi
Copy link
Contributor Author

dkarlovi commented Oct 9, 2024

Let me look for another one to work on

Great, there's not a shortage of work on the project(s)! 😄

@RichardChukwu
Copy link

RichardChukwu commented Oct 11, 2024

Hello @dkarlovi please can you specify the exact folder location for this. I can't seem to figure out here symfony auto-instrumentation is in the repo inorder to apply a fix

@dkarlovi
Copy link
Contributor Author

@RichardChukwu I know what you mean, I have the same issue navigating these OTEL repos, it's very confusing if you're not here day to day, I was about to suggest the contrib repo gets merged into this repo since the packages are now released from the split repos anyway. WDYT @bobstrecansky @brettmc?

To answer your question, it's here I believe:
https://github.com/open-telemetry/opentelemetry-php-contrib/tree/main/src/Instrumentation/Symfony

@RichardChukwu
Copy link

RichardChukwu commented Oct 11, 2024

Hello @brettmc kindly review PR (open-telemetry/opentelemetry-php-contrib#304) as a fix I submitted to this issue. I'll appreciate feedback where necessary too. Thank you

@dkarlovi
Copy link
Contributor Author

dkarlovi commented Oct 16, 2024

@bobstrecansky I believe this issue shouldn't be closed because

it seems the consumer is currently not instrumented at all
it seems to me the names of the spans are also not correct

The linked PR changed spans to KIND_PRODUCER, but KIND_CONSUMER wasn't updated (since it doesn't even exist, as noted) and the names weren't updated to match the spec either.

@brettmc brettmc reopened this Oct 16, 2024
@brettmc
Copy link
Collaborator

brettmc commented Oct 16, 2024

@RichardChukwu some additional changes noted above, if you're interested in working on them :)

@RichardChukwu
Copy link

Before working on this, just want to ensure i get you correctly @dkarlovi

Do you mean updating the code to introduce new spans with KIND_CONSUMER for the parts of the code handling message reception or processing as it doesn't exist?

For instance, based on the spec from what I can see, I presume key changes will be:

  • Adding the "consume" operation with SpanKind::KIND_CONSUMER for receiving messages.

  • Updating span names to use publish, send, and consume as per the spec, ensuring naming consistency for operations.

  • Instrumenting the SenderInterface for both sending and receiving spans, with the appropriate SpanKind (producer for sending and consumer for receiving).

Your thoughts please....

@dkarlovi
Copy link
Contributor Author

Do you mean updating the code to introduce new spans with KIND_CONSUMER for the parts of the code handling message reception or processing as it doesn't exist?

Yes, correct. The messages being created and sent is currently fully instrumented, but them being received and processed is not instrumented at all, from my understanding at least.

@RichardChukwu
Copy link

Alright, got it. I'll work on that and send a PR for a fix soon @dkarlovi

@RichardChukwu
Copy link

RichardChukwu commented Oct 17, 2024

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working good first issue Good for newcomers help wanted This issue is looking for someone to work on it
Projects
None yet
6 participants