Skip to content

Conversation

bhawnapannu2701
Copy link

The existing RabbitMQ tests cover legacy network attributes. The spec now defines new network keys (e.g., network.peer.address/port, server.address/port). This test publishes and consumes a message and asserts that recorded spans contain at least one of the new attributes, increasing coverage without touching production code.

• Test-only change, no production code touched
• Runs locally with: gradlew test -p instrumentation/rabbitmq-2.7/javaagent
• Formatted via spotlessApply

@bhawnapannu2701 bhawnapannu2701 requested a review from a team as a code owner September 22, 2025 16:01
Copy link

linux-foundation-easycla bot commented Sep 22, 2025

CLA Signed

The committers listed above are authorized under a signed CLA.

*
* <p>Test-only; no production code changed.
*/
public class RabbitMqNewNetAttributesTest {
Copy link
Member

Choose a reason for hiding this comment

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

Is there any reason not to make changes in the existing test classes where it already has all the container setup code etc?

There are already some assertions for network attributes, if you think there is something missing, you could look to update things there instead of a new class?

@bhawnapannu2701 bhawnapannu2701 force-pushed the rabbitmq-new-network-test branch from 7b9b238 to dd5def2 Compare September 24, 2025 05:19
@otelbot-java-instrumentation
Copy link
Contributor

🔧 The result from spotlessApply was committed to the PR branch.

@bhawnapannu2701
Copy link
Author

Thanks for the feedback! I moved the assertions into the existing RabbitMqTest (reusing container/setup) and removed the extra class. The updated verifyNetAttributes accepts either legacy (network.peer.) or the newer semconv (server./client.*) keys via AttributeKey, so it passes under both configurations without production changes.

@laurit
Copy link
Contributor

laurit commented Sep 24, 2025

We don't implement the latest version of the messaging semantic conventions and thus don't emit attributes like server.address.

@bhawnapannu2701
Copy link
Author

All checks have passed and the branch is up to date with my latest changes.
Please let me know if you’d like me to rebase or make any further edits.

@laurit
Copy link
Contributor

laurit commented Sep 24, 2025

All checks have passed and the branch is up to date with my latest changes. Please let me know if you’d like me to rebase or make any further edits.

@bhawnapannu2701 I'm sorry but we won't be merging this PR since the instrumentation does not emit these attributes. When the instrumentation is update we'll be using a flag like otel.semconv-stability.opt-in=messaging In the instrumentation code we'll emit the attributes based on the flag

if (SemconvStability.emitStableDatabaseSemconv()) {
internalSet(attributes, DB_QUERY_TEXT, getter.getDbQueryText(request));
internalSet(attributes, DB_OPERATION_NAME, getter.getDbOperationName(request));
}
if (SemconvStability.emitOldDatabaseSemconv()) {
internalSet(attributes, DB_STATEMENT, getter.getDbQueryText(request));
internalSet(attributes, DB_OPERATION, getter.getDbOperationName(request));
}
and in the test we also choose which attribute to assert on based on a flag
If you wish to work on updating the messaging attributes try to follow the established conventions used for updating the database conventions. Since this is going to be large project and will also involve changes in the span structure my advice would be for you to start small. Don't attempt to do everything in one PR.

@laurit laurit added the needs author feedback Waiting for additional feedback from the author label Sep 24, 2025
Copy link
Contributor

github-actions bot commented Oct 1, 2025

This PR has been labeled as stale due to lack of activity and needing author feedback. It will be automatically closed if there is no further activity over the next 7 days.

@github-actions github-actions bot added the stale label Oct 1, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs author feedback Waiting for additional feedback from the author stale
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants