fix: dns plugin remove hostname attribute

[svrnm]

Which problem is this PR solving?

In the dns plugin the net.peer.name is used for the looked up hostname, which is not the peer we are connecting to (that would be the DNS server if a real lookup would be executed)

Short description of the changes

• Replace net.peer.name in src and test with a custom attribute dns.hostname

[codecov]

Codecov Report

Merging #488 (bc7361d) into main (8453034) will decrease coverage by 0.00%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main     #488      +/-   ##
==========================================
- Coverage   95.40%   95.40%   -0.01%     
==========================================
  Files         144      144              
  Lines        8664     8661       -3     
  Branches      812      812              
==========================================
- Hits         8266     8263       -3     
  Misses        398      398              

Impacted Files	Coverage Δ
...lemetry-instrumentation-dns/src/instrumentation.ts	76.38% <0.00%> (-0.33%)	⬇️
...metry-instrumentation-dns/test/utils/assertSpan.ts	100.00% <0.00%> (ø)

[dyladan]

Do we even want to save the looked up host as an attribute at all? This would be like saving the http response as an attribute for an HTTP span. I would maybe prefer to remove it entirely.

There are no DNS semantic conventions at all because I think most instrumentations consider the DNS as a part of the HTTP/GRPC/etc request span rather than as an entirely separate operation to be traced.

[svrnm]

Do we even want to save the looked up host as an attribute at all? This would be like saving the http response as an attribute for an HTTP span. I would maybe prefer to remove it entirely.

Interesting point. I tried to come up with some use cases for it, but had a hard time:

• DNS Errors -> this would be covered by the error being thrown
• "Analyzing and Optimising DNS" -> how often is DNS really slow or making trouble? Could be the case, but the hostname is not required for that
• Services that use DNS independently to achieve something and want to have telemetry. But in that case dns.lookup(...) is probably not used.

None of them is really convincing, maybe since the code is already there, adding an option to collect that attribute is a compromise? But I can also see this being removed.

There are no DNS semantic conventions at all because I think most instrumentations consider the DNS as a part of the HTTP/GRPC/etc request span rather than as an entirely separate operation to be traced.

The thing I like about having a dns instrumentation is the capability of splitting a HTTP/GDPR/... call into like the browser provides it. But since the hostname should be the same as net.peer.name it's not the important piece of information in the case of an issue. The resolved IP address & family might be helpful, maybe also the source of the information (hosts file, DNS server,...)

[dyladan]

I would honestly prefer to remove it. If someone needs it we can add it back later. In extreme cases the hostname can even be considered private information.

The thing I like about having a dns instrumentation is the capability of splitting a HTTP/GDPR/... call into like the browser provides it. But since the hostname should be the same as net.peer.name it's not the important piece of information in the case of an issue. The resolved IP address & family might be helpful, maybe also the source of the information (hosts file, DNS server,...)

This seems like the perfect thing to take to the spec

[svrnm]

I would honestly prefer to remove it. If someone needs it we can add it back later. In extreme cases the hostname can even be considered private information.

Agreed. I removed it for now.

[moander]

It would be nice to have a config option to include the hostname.

[evantorrie]

And also a requireParent flag (see this discussion for prior art)