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.

Sign up for GitHub

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

Jump to bottom

Change OTel attribute messaging.operation to messaging.operation.type #1716

Merged
merged 5 commits into from
Nov 1, 2024
Merged

Change OTel attribute messaging.operation to messaging.operation.type #1716

merged 5 commits into from
Nov 1, 2024

Conversation

iinuwa
Copy link
Contributor

@iinuwa iinuwa commented Oct 30, 2024 •
edited by michaelklishin
Loading

Proposed Changes

Update messaging.operation to messaging.operation.type to comply with OpenTelemetry Semantic Conventions for Messaging.

Addresses part of #1715.
References #1528.

Types of Changes

What types of changes does your code introduce to this project?
Put an x in the boxes that apply

  • Bug fix (non-breaking change which fixes issue #NNNN)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause an observable behavior change in existing systems)
  • Documentation improvements (corrections, new content, etc)
  • Cosmetic change (whitespace, formatting, etc)

Checklist

Put an x in the boxes that apply. You can also fill these out after creating
the PR. If you're unsure about any of them, don't hesitate to ask on the
mailing list. We're here to help! This is simply a reminder of what we are
going to look for before merging your code.

  • I have read the CONTRIBUTING.md document
  • I have signed the CA (see https://cla.pivotal.io/sign/rabbitmq)
  • All tests pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)
  • Any dependent changes have been merged and published in related repositories

Further Comments

This is a breaking change to the OpenTelemetry API, FWIW.

@iinuwa
Copy link
Contributor Author

iinuwa commented Oct 30, 2024 •
edited
Loading

This is separate issue, but there were a couple problems with building this:

  • There is no NuGet.config file specified in the repository, which causes the build to fail if your local development machine has any private/alternate repositories configured. Consider adding a blank NuGet.config to the root of the repository, like:
<?xml version="1.0" encoding="utf-8"?>
<configuration>
  <packageSources>
    <clear/>
    <add key="nuget.org" value="https://api.nuget.org/v3/index.json" />
  </packageSources>
</configuration>
  • I also encountered an issue building because the "toxiproxy" project is not checked in, so I couldn't run dotnet build ./Build.csproj && make test successfully.

michaelklishin
michaelklishin previously approved these changes Oct 30, 2024
View reviewed changes
@iinuwa iinuwa force-pushed the fix/change-otel-attribute-name branch from 1b334d7 to 381b29d Compare October 30, 2024 16:22
@iinuwa iinuwa mentioned this pull request Oct 30, 2024
Open
@iinuwa
Copy link
Contributor Author

iinuwa commented Oct 30, 2024

I don't know if the values are also up for changing here, but we're also using deprecated values:

publish -> send
deliver -> process

@iinuwa
Copy link
Contributor Author

iinuwa commented Oct 30, 2024

(sorry for the force push, I was a little too trigger happy on the create PR button)

@michaelklishin
Copy link
Member

@iinuwa no worries, as long as the end result of a PR looks like what you want, you can force push all you want on topic branches.

@michaelklishin
Copy link
Member

@iinuwa please update the enum values to send and process. Again, now (before 7.0.0 has shipped) is the best moment to do that.

@iinuwa
Copy link
Contributor Author

iinuwa commented Oct 30, 2024

@michaelklishin Sorry, I didn't see an enum or const string for the old publish or deliver values. I did update the strings values to send and process though.

Did you want me to add a new set of const strings for these?

Do you also want to rename RabbitMqActivitySource.Deliver() to RabbitMqActivitySource.Process()? (I would think not since "Deliver" is used everywhere else in the client, and we wouldn't the OTel stuff to leak everywhere, but up to you.)

@michaelklishin
Copy link
Member

michaelklishin commented Oct 30, 2024 •
edited
Loading

@iinuwa I did not mean "an enum" in the C# sense but rather in an abstract sense. The accepted values of message "types" are well known. We need to switch to the recently recommended ones before 7.0 ships. I think it can be a part of this PR.

@michaelklishin
Copy link
Member

michaelklishin commented Oct 30, 2024 •
edited
Loading

As for renaming RabbitMqActivitySource.Deliver to RabbitMqActivitySource.Process, I'm not sure. The latter does not make it clear to me what it returns but we might be forced to follow these OpenTelemetry conventions.

OpenTelemetry support was originally contributed in #1528 by @stebet, @lmolkova and others. Perhaps they would have an opinion.

@iinuwa
Copy link
Contributor Author

iinuwa commented Oct 30, 2024

I think I was confused since the values were already updated in this PR (I think you may have reviewed before I pushed those changes). I've added the const strings too to make them more apparent.

@michaelklishin
Copy link
Member

@iinuwa this does not update relevant tests, for example, the Test.SequentialIntegration.TestOpenTelemetry.AssertActivityData failure on Actions seems legit (not a flake) and relevant to me.

@lukebakken lukebakken self-requested a review October 30, 2024 17:27
@lukebakken lukebakken self-assigned this Oct 30, 2024
@lukebakken lukebakken added this to the 7.0.0 milestone Oct 30, 2024
@lukebakken
Copy link
Contributor

I also encountered an issue building because the "toxiproxy" project is not checked in, so I couldn't run dotnet build ./Build.csproj && make test successfully.

@iinuwa you must clone with submodules, or update submodules afterward.

@lukebakken
Copy link
Contributor

I am pinging these people because they all reviewed #1528 -

@danielmarbach
@lmolkova
@martinjt
@Kielek
@cijothomas
@WeihanLi

lukebakken added a commit that referenced this pull request Oct 30, 2024
@lukebakken
Add NuGet.config
e1b62d1 
Source:
https://github.com/npgsql/npgsql/blob/main/NuGet.config

As suggested here:
#1716 (comment)
@lukebakken lukebakken force-pushed the fix/change-otel-attribute-name branch from 1eefb59 to ab67b7b Compare October 30, 2024 17:48
@lukebakken
Copy link
Contributor

@iinuwa as @michaelklishin says here, please fix the test suite. Thank you for your contribution.

@iinuwa
Copy link
Contributor Author

iinuwa commented Oct 30, 2024

Done, thank you! Sorry for the confusion; I always forget about git submodules, and it wasn't mentioned in the CONTRIBUTING.md doc. That made me able to run the tests locally.

@lukebakken
Copy link
Contributor

and it wasn't mentioned in the CONTRIBUTING.md doc.

Feel free to modify that file in this PR to mention cloning with submodules. Thanks.

@iinuwa
Copy link
Contributor Author

iinuwa commented Oct 30, 2024

Took a stab at updating the docs.

@lukebakken lukebakken force-pushed the fix/change-otel-attribute-name branch from 94f825f to acb3515 Compare October 30, 2024 18:21
@lukebakken
Copy link
Contributor

Thank you for the review @danielmarbach

WeihanLi
WeihanLi approved these changes Oct 30, 2024
View reviewed changes
Copy link
Contributor

@WeihanLi WeihanLi left a comment

Choose a reason for hiding this comment

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

LGTM

lmolkova
lmolkova reviewed Oct 31, 2024
View reviewed changes
projects/RabbitMQ.Client/Impl/RabbitMQActivitySource.cs
@@ -63,14 +66,14 @@ public static class RabbitMQActivitySource

Activity? activity = linkedContext == default
? s_publisherSource.StartRabbitMQActivity(
UseRoutingKeyAsOperationName ? $"{routingKey} publish" : "publish",
UseRoutingKeyAsOperationName ? $"{routingKey} {MessagingOperationTypeSend}" : MessagingOperationTypeSend,
Copy link

Choose a reason for hiding this comment

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

span name should be {operation name} {destination} - https://github.com/open-telemetry/semantic-conventions/blob/main/docs/messaging/messaging-spans.md#span-name.

I.e. publish stays (as rabbit-friendly operation name), but the order has changed and should now be publish {routingKey}

Or if you want to call it basic_publish instead of publish or anything else (language-agnostic) - go for it - happy to update otel semconv with your preference.

projects/RabbitMQ.Client/Impl/RabbitMQActivitySource.cs
@@ -85,12 +88,12 @@ public static class RabbitMQActivitySource
}

Activity? activity = s_subscriberSource.StartRabbitMQActivity(
UseRoutingKeyAsOperationName ? $"{queue} receive" : "receive",
UseRoutingKeyAsOperationName ? $"{queue} {MessagingOperationTypeReceive}" : MessagingOperationTypeReceive,
Copy link

Choose a reason for hiding this comment

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

Is this activity created for BasicGet ? If so, let's pick a rabbitMQ-friendly operation name for it.

See my comment on publish.

Also, the order has changed - it's now {operation_name} {destination}

projects/RabbitMQ.Client/Impl/RabbitMQActivitySource.cs
ActivityKind.Producer, linkedContext);
if (activity != null && activity.IsAllDataRequested)
{
PopulateMessagingTags("publish", routingKey, exchange, 0, bodySize, activity);
PopulateMessagingTags(MessagingOperationTypeSend, routingKey, exchange, 0, bodySize, activity);
Copy link

Choose a reason for hiding this comment

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

we'll need to set operation name too - it's required now.

projects/RabbitMQ.Client/Impl/RabbitMQActivitySource.cs
ActivityKind.Consumer);
if (activity != null && activity.IsAllDataRequested)
{
activity
.SetTag(MessagingOperation, "receive")
.SetTag(MessagingOperationType, MessagingOperationTypeReceive)
Copy link

Choose a reason for hiding this comment

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

Suggested change
.SetTag(MessagingOperationType, MessagingOperationTypeReceive)
.SetTag(MessagingOperationName, "something that we'll pick for `BasicGet``")

projects/RabbitMQ.Client/Impl/RabbitMQActivitySource.cs
@@ -107,11 +110,11 @@ public static class RabbitMQActivitySource

// Extract the PropagationContext of the upstream parent from the message headers.
Activity? activity = s_subscriberSource.StartLinkedRabbitMQActivity(
UseRoutingKeyAsOperationName ? $"{routingKey} receive" : "receive", ActivityKind.Consumer,
UseRoutingKeyAsOperationName ? $"{routingKey} {MessagingOperationTypeReceive}" : MessagingOperationTypeReceive, ActivityKind.Consumer,
Copy link

Choose a reason for hiding this comment

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

same here

projects/RabbitMQ.Client/Impl/RabbitMQActivitySource.cs
@@ -128,11 +131,11 @@ public static class RabbitMQActivitySource

// Extract the PropagationContext of the upstream parent from the message headers.
Activity? activity = s_subscriberSource.StartLinkedRabbitMQActivity(
UseRoutingKeyAsOperationName ? $"{routingKey} deliver" : "deliver",
UseRoutingKeyAsOperationName ? $"{routingKey} {MessagingOperationTypeProcess}" : MessagingOperationTypeProcess,
Copy link

Choose a reason for hiding this comment

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

this one is probably for consume, so let's do consume|basic_consume|... {routing_key} and add the operation name tag with the name we'll pick

@iinuwa iinuwa mentioned this pull request Oct 31, 2024
Open
11 tasks
@lukebakken lukebakken force-pushed the fix/change-otel-attribute-name branch from acb3515 to e2c246f Compare October 31, 2024 21:37
@lukebakken
Copy link
Contributor

@iinuwa is there anything you plan to change in this PR or is it ready to merge? I assigned #1717 to the 7.1.0 milestone.

@iinuwa
Copy link
Contributor Author

iinuwa commented Oct 31, 2024 via email

@michaelklishin michaelklishin merged commit 4525c6d into rabbitmq:main Nov 1, 2024
11 checks passed
@michaelklishin
Copy link
Member

Thank you. Let's continue in #1717.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lmolkova lmolkova lmolkova left review comments

@WeihanLi WeihanLi WeihanLi approved these changes

@danielmarbach danielmarbach danielmarbach approved these changes

@michaelklishin michaelklishin michaelklishin approved these changes

@lukebakken lukebakken Awaiting requested review from lukebakken

Assignees

@lukebakken lukebakken

Labels
None yet
Projects
None yet
Milestone
7.0.0
Development

Successfully merging this pull request may close these issues.
6 participants
@iinuwa @michaelklishin @lukebakken @danielmarbach @lmolkova @WeihanLi