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

Database: clarify network attributes usage on common semconv #768

Merged
merged 6 commits into from
Mar 18, 2024

Conversation

lmolkova
Copy link
Contributor

@lmolkova lmolkova commented Feb 23, 2024

Changes

Addresses DB portion of #690:

  • Removes network.transport|type attributes from general db semantic conventions.
  • Clarifies when specific systems should set network.server.address|port and keeps them on the cassandra, redis and elasticsearch extensions

Based on the findings from Java instrumentation libraries, following instrumentations populate network attributes:

  • cassandra
  • redis (Redisson, jedis, lettuce, vertx, redis)
  • elastic
  • opensearch

See #698 (comment) for more details.

Merge requirement checklist

@lmolkova lmolkova requested review from a team as code owners February 23, 2024 23:58
@lmolkova lmolkova changed the title Database: remove network attribute from general, keep for some extensions Database: remove network attributes from general semconv Feb 28, 2024
@trask
Copy link
Member

trask commented Feb 28, 2024

Based on the findings from Java instrumentation libraries, following instrumentations populate network attributes:

  • cassandra
  • redis (Redisson, jedis, lettuce, vertx, redis)
  • elastic
  • opensearch

do you think this is "because the instrumentation can", or because there is something about these databases that makes us include network.peer.address in addition to server.address?

what I'm trying to understand is what's the criteria for including (or excluding) network.peer.address in tech-specific database semconv

@lmolkova
Copy link
Contributor Author

lmolkova commented Mar 7, 2024

Based on the findings from Java instrumentation libraries, following instrumentations populate network attributes:

  • cassandra
  • redis (Redisson, jedis, lettuce, vertx, redis)
  • elastic
  • opensearch

do you think this is "because the instrumentation can", or because there is something about these databases that makes us include network.peer.address in addition to server.address?

what I'm trying to understand is what's the criteria for including (or excluding) network.peer.address in tech-specific database semconv

I can identify one good criteria - self-hosting (or hosting model where users configure/scale individual nodes).
Cassandra, Redis, Elastic and OpenSearch are in this camp (even though there are fully-managed offerings for some of them).

As a user who hosts cassandra, redis, elastic, etc clusters, I'm much more interested in which specific node this operation was performed on as it helps me detect issues with that specific node/my own infra, detect sharding/replication misconfiguration, etc.

If I use fully managed DB (DBaaS), specific node information is probably missing (even if I have an IP address, it's probably a public one from load balancer or NAT) and not actionable. Basic sharding/replication is handled by my vendor to a big extent and not visible to my application.

Update: mongoDB also supports self-hosted mode and might also be a candidate to have network.peer.* attributes

@lmolkova
Copy link
Contributor Author

lmolkova commented Mar 8, 2024

Update: based on the DB WG discussion on 3/8 the consensus is to keep network.peer.* in the general semconv with something like recommended: if applicable level

@lmolkova lmolkova force-pushed the db-network-remove branch 3 times, most recently from dc3fd67 to e45298a Compare March 8, 2024 23:50
@lmolkova lmolkova changed the title Database: remove network attributes from general semconv Database: clarify network attributes usage on common semconv Mar 9, 2024
model/trace/database.yaml Outdated Show resolved Hide resolved
model/trace/database.yaml Outdated Show resolved Hide resolved
@joaopgrassi joaopgrassi merged commit f9dfdd5 into open-telemetry:main Mar 18, 2024
10 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.

None yet

4 participants