search

open-telemetry / opentelemetry-go-contrib

Collection of extensions for OpenTelemetry-Go.
https://opentelemetry.io/
Apache License 2.0
1.18k stars 555 forks source link

[otelmongo] should use `AttributeNetPeerIP` when hostname is a IP #2165

Open jixiuf opened 2 years ago

jixiuf commented 2 years ago

https://github.com/open-telemetry/opentelemetry-go-contrib/blob/1e34aa359ff6cfe4ba2a44775d261311ea024197/instrumentation/go.mongodb.org/mongo-driver/mongo/otelmongo/mongo.go#L54

should use semconv.NetPeerIPKey when hostname is a IP

DiptoChakrabarty commented 2 years ago

Hi @jixiuf can I take this up please

alamzeeshan commented 1 year ago

@jixiuf This looks abandoned. Can I take this up? Please assign this to me. I will open a PR along with the relevant unit tests.

alamzeeshan commented 1 year ago

@evantorrie @jmacd @XSAM Can I take this up?

dmathieu commented 1 year ago

Feel free to open a PR and link this issue on it.

carrbs commented 10 months ago

@pellared, @dmathieu, I can pick this up since it looks abandoned, but a few questions:

  1. is this still needed?
  2. Based on what I see in the changelog, and the NOTE here it looks like we'd set NetSockPeerAddr if hostname is an IP address, otherwise set NetPeerName?
  3. which unit tests are you referring to?

https://github.com/open-telemetry/opentelemetry-specification/blob/bb3d0a0d14f41ccd3f78852316c77705e722edfa/CHANGELOG.md?plain=1#L1126-L1127

https://github.com/open-telemetry/opentelemetry-go/blob/d37d851bbce65577ab340849ec6c540fdc6e3096/semconv/v1.17.0/trace.go#L1115-L1117

pellared commented 10 months ago

There is a plan to deprecate the module and move the ownership to MongoDB team.

I do not think it is worth to take the time in fix as there are no codeowners for this module so there is low chance that the PR will get a review.

Reference: https://jira.mongodb.org/browse/GODRIVER-2938

pellared commented 10 months ago

Blocked by https://github.com/open-telemetry/opentelemetry-go-contrib/issues/4678 (no codeowners).

prestonvasquez commented 3 weeks ago

@pellared This issue can be closed as if https://github.com/open-telemetry/opentelemetry-go-contrib/pull/6172 is accepted. semconv/v1.26.0 deprecates net.peer.ip in favor of network.peer.address.