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

Messaging: clarify network attributes usage on common semconv #698

Merged
merged 14 commits into from
Mar 25, 2024

Conversation

lmolkova
Copy link
Contributor

@lmolkova lmolkova commented Feb 7, 2024

Changes

Addresses messaging portion of #690

Removes network.transport|type attributes from general messaging semantic conventions.

Based on the findings from Java instrumentation libraries, the only messaging instrumentation that populates network attributes is RabbitMQ (which also sets them in native .NET instrumentation).

This PR clarifies that network.server.address|port should be only set when they represent a specific broker instance (when individual instances are visible from the application side) and recommends to use them on the RabbitMQ (as one of the systems that's usually self-hosted)

Merge requirement checklist

@lmolkova lmolkova mentioned this pull request Feb 7, 2024
3 tasks
@pyohannes
Copy link
Contributor

From the messaging point of view, I'd agree to merge those changes.

@lmolkova lmolkova marked this pull request as ready for review February 13, 2024 05:16
@lmolkova lmolkova requested review from a team February 13, 2024 05:16
model/trace/database.yaml Outdated Show resolved Hide resolved
@lmolkova lmolkova force-pushed the db-messaging-no-network branch from f1fabe3 to 4cae674 Compare February 15, 2024 05:59
@lmolkova lmolkova marked this pull request as draft February 21, 2024 04:39
Copy link
Member

@trask trask left a comment

Choose a reason for hiding this comment

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

it would be great to look over the existing Java database instrumentations to see how many places where we're (usefully/correctly?) capturing network attributes

@lmolkova
Copy link
Contributor Author

lmolkova commented Feb 22, 2024

Looking into java instrumentation repo:

Messaging systems:

  • report only network attributes:
  • report server and network attributes:
    • aws sns/sqs server and only network.protocol.verion out of all network attributes
  • report server attributes only:
    • pulsar
  • report no server/network attributes at all: kafka, rocketmq

Databases:

  • report only network:
    • cassandra
    • Redisson
  • report only server:
    • jdbc
    • mongo
    • r2
    • vertxsql
    • elastic rest
  • report both, server and network attributes:
    • dynamodb (also reports network.protocol.verion, but no other network)
    • jedis (depends on the version)
    • lettuce
    • vertx redis
    • elastic-transport
    • opensearch
  • report nothing (no network or server): geodedb, Rediscala, Spymemcached

All instrumentations that report network, report only network.peer.address|port and some report network.type. I could not find any that reports protocol name or transport.

I probably missed something, but I think there is enough evidence.

  • server.* or network attributes cannot be required on DB or messaging
  • I think instrumentations that support network, but don't report server.address should try by calling getHostName() on Inet*Address they already have
  • I still want to remove network from general db, but keep it for redis, cassandra, and elastic.
  • I still want to remove network from general messaging, but want to keep them for rabbitmq

@pyohannes
Copy link
Contributor

Looking into java instrumentation repo:

Thanks @lmolkova for this research.

To add one more datapoint, the .NET RabbitMQ instrumentation populates both network and server attributes (if they are available):

  • network.peer.address
  • network.peer.port
  • network.local.address
  • network.local.port
  • network.type
  • network.protocol.name
  • network.protocol.version
  • server.address
  • server.port

@lmolkova lmolkova force-pushed the db-messaging-no-network branch from 5dd535a to 11df88f Compare February 23, 2024 23:01
@lmolkova lmolkova changed the title Remove network attributes from messaging and database semconv Remove network attributes from messaging semconv Feb 23, 2024
@lmolkova lmolkova marked this pull request as ready for review February 23, 2024 23:07
@lmolkova
Copy link
Contributor Author

Based on the discussion in Messaging SIG, breaking this one down into two PRs. This one contains messaging portion only.

Will send a separate one for DBs.

@lmolkova lmolkova force-pushed the db-messaging-no-network branch 2 times, most recently from efb2771 to 405e120 Compare March 2, 2024 00:07
@lmolkova lmolkova force-pushed the db-messaging-no-network branch from 9b9e055 to 823e6e0 Compare March 9, 2024 00:12
@lmolkova lmolkova changed the title Remove network attributes from messaging semconv Messaging: clarify network attributes usage on common semconv Mar 9, 2024
@lmolkova
Copy link
Contributor Author

lmolkova commented Mar 9, 2024

Based on the database discussion on similar PR #768 (comment), I updated this one to be consistent with databases:

  • removed network.transport|type
  • kept network.peer.* in common, but clarified they apply for self-hosted systems where they make sense

@open-telemetry/semconv-messaging-approvers please re-review

@lmolkova lmolkova force-pushed the db-messaging-no-network branch from 823e6e0 to cd68332 Compare March 11, 2024 18:44
@lmolkova lmolkova force-pushed the db-messaging-no-network branch 2 times, most recently from b3b354e to 4594a7f Compare March 14, 2024 22:17
model/trace/messaging.yaml Outdated Show resolved Hide resolved
model/trace/messaging.yaml Outdated Show resolved Hide resolved
@lmolkova lmolkova force-pushed the db-messaging-no-network branch from 8f1338f to 7afa1b6 Compare March 18, 2024 17:47
docs/messaging/rabbitmq.md Outdated Show resolved Hide resolved
@lmolkova lmolkova merged commit e01cafd into open-telemetry:main Mar 25, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

6 participants