Skip to content

Use default service name for RabbitMQ consumer - #855

Merged
mar-kolya merged 1 commit into
masterfrom
mar-kolya/rabbitmq-client-service-name
May 24, 2019
Merged

mar-kolya merged 1 commit into
masterfrom
mar-kolya/rabbitmq-client-service-name

Conversation

@mar-kolya

Copy link
Copy Markdown
Contributor

So spans parented by consumer span had reasonable service name

@mar-kolya
mar-kolya requested review from labbati and tylerbenson May 24, 2019 00:34

@labbati labbati left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please see comment on #854
If the answer to that is 'no' then this can be merged for me.

/*
Use default service name. Common use-case here is to have consumer span parent
children spans in instrumented application. Since service name is inherited it makes
sense to default that to application service name rather than 'kafka'.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is rabbitmq?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed, thanks!

So spans parented by consumer span had reasonable service name
@mar-kolya
mar-kolya force-pushed the mar-kolya/rabbitmq-client-service-name branch from 932fafb to a7271ed Compare May 24, 2019 13:06

@labbati labbati left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 for me

@mar-kolya
mar-kolya merged commit 82f1a48 into master May 24, 2019
@mar-kolya
mar-kolya deleted the mar-kolya/rabbitmq-client-service-name branch May 24, 2019 14:51
@brentm5

brentm5 commented May 24, 2019 •

Copy link
Copy Markdown
Contributor

I created #858 to help provide potentially a better path forward for the default rabbitmq consumer implementations. I have some concerns around what the above would do for metrics / alerts on the rabbitmq service. By doing this you would just move deliver methods to not show up as rabbitmq and instead the service.

@tylerbenson tylerbenson added this to the 0.29.0 milestone May 29, 2019
tylerbenson added a commit that referenced this pull request May 29, 2019
These are breaking changes that need more vetting.
tylerbenson added a commit that referenced this pull request May 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants