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

Add Dns event counters #48885

Open
stephentoub opened this issue Feb 28, 2021 · 17 comments
Open

Add Dns event counters #48885

stephentoub opened this issue Feb 28, 2021 · 17 comments
Labels
area-System.Net enhancement Product code improvement that does NOT require public API changes/additions
Milestone

Comments

@stephentoub
Copy link
Member

We should consider adding some event counters around name resolution, e.g.

  • Number of Dns requests issued
  • Number of Dns requests currently in fight
  • Number of unique Dns requests currently in flight
@stephentoub stephentoub added this to the 6.0.0 milestone Feb 28, 2021
@ghost
Copy link

ghost commented Feb 28, 2021

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

We should consider adding some event counters around name resolution, e.g.

  • Number of Dns requests issued
  • Number of Dns requests currently in fight
  • Number of unique Dns requests currently in flight
Author: stephentoub
Assignees: -
Labels:

area-System.Net

Milestone: 6.0.0

@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged New issue has not been triaged by the area owner label Feb 28, 2021
@stephentoub stephentoub changed the title Dns event counters Add Dns event counters Feb 28, 2021
@MihaZupan
Copy link
Member

MihaZupan commented Mar 1, 2021

Right now we have

  • dns-lookups-requested - DNS Lookups Requested (total counter)
  • dns-lookups-duration - Average DNS Lookup Duration

Number of Dns requests currently in fight

Something like current-dns-lookups seems reasonable - we have that for Net.Security and Net.Http as well.

With #48566 in mind, the meaning of this would likely be "number of not-finished Dns.Foo(...) calls".

Number of unique Dns requests currently in flight

I assume the tracking of "unique" would be duplicated between the Telemetry impl and a potential #48566 impl?

@stephentoub
Copy link
Member Author

dns-lookups-requested

Ah, I didn't see that one. That's the "Number of Dns requests issued" I was asking for.

Something like current-dns-lookups seems reasonable ... the meaning of this would likely be "number of not-finished Dns.Foo(...) calls"

Yes, if we're already tracking number of requests initiated, then if we also tracked number of requests finished, it would be the difference.

I assume the tracking of "unique" would be duplicated between the Telemetry impl and a potential #48566 impl?

There might be some duplication, but whatever we do for #48566 is likely to only apply to a portion of requests, e.g. those that are implemented by queueing work items to the thread pool. On Windows most if not all requests will instead be implemented with async OS functionality, and we wouldn't do any such uniqueness tracking for the implementation. Longer term, we'd expect that to be true for all OSes.

@ManickaP
Copy link
Member

ManickaP commented Mar 4, 2021

Triage: shouldn't be hard to implement, the numbers might be useful. We would want current-dns-lookups for 6.0.

@ManickaP ManickaP removed the untriaged New issue has not been triaged by the area owner label Mar 4, 2021
@karelz karelz added the enhancement Product code improvement that does NOT require public API changes/additions label May 4, 2021
@karelz karelz modified the milestones: 6.0.0, Future May 6, 2021
@MihaZupan
Copy link
Member

MihaZupan commented Jul 21, 2021

Currently implemented:

  • Start/Failed/Stop events
    • Failed could use some additional info such as the exception message - right now it's just an empty event
  • dns-lookups-requested (total for process)
  • current-dns-lookups (current)
  • dns-lookups-duration (average time taken for lookups within the last counter period)

Not yet added:

  • Number of unique DNS requests currently in flight

Potential:

@karelz
Copy link
Member

karelz commented Jul 21, 2021

Taking the discussion from #56072 (comment) here:

How many unique DNS addresses are resolved in an application’s lifetime?

What is a developer going to do with this information? Doing this "perfectly" would be a leak, as it would require the application to maintain a set of every address it saw; that's not viable (it'd be like Kestrel having a counter for how many unique client addresses connected to it over the process' lifetime). Events are already emitted for that information, so a dev can post-process that information from traces if needed. #48885 (this issue) proposes one for the number of unique addresses currently being resolved; from my perspective, that's more reasonable, though I'm not sure how actionable it actually is, either.

We should answer:

  1. Do we want current vs. app-lifetime counter?
  2. How is it useful to end users?
  3. Is it important for 6.0?

@davidfowl can you respond here please to @stephentoub's comment above?

Moving the issue to 6.0 to make decision what to do in 6.0.

@karelz karelz modified the milestones: Future, 6.0.0 Jul 21, 2021
@danmoseley
Copy link
Member

danmoseley commented Jul 21, 2021

There are ways to get a statistical estimate of the number of unique addresses with quite limited storage.

https://stats.stackexchange.com/questions/19014/how-can-i-estimate-unique-occurrence-counts-from-a-random-sampling-of-data

https://en.wikipedia.org/wiki/HyperLogLog?wprov=sfti1 (there is a relatively simple C# implementation with Apache license that could be used.

https://en.wikipedia.org/wiki/Count-distinct_problem?wprov=sfti1

Etc

@geoffkizer
Copy link
Contributor

Events are already emitted for that information, so a dev can post-process that information from traces if needed

+1. Let's log data at runtime and do statistical analysis in post-processing.

@karelz
Copy link
Member

karelz commented Jul 26, 2021

Moving it to 7.0. @davidfowl let us know if you have more info that should change our mind ...

@karelz karelz modified the milestones: 6.0.0, 7.0.0 Jul 26, 2021
@MihaZupan MihaZupan removed their assignment Jul 26, 2021
@danmoseley
Copy link
Member

@davidfowl can you ack or push back here to help us make the right call?

@davidfowl
Copy link
Member

davidfowl commented Aug 5, 2021

Is there any low hanging fruit here we could tackle for .NET 6? I think the unique dns requests are difficult and we can punt for now, but it would be ideal to add some of the other counters.

@MihaZupan
Copy link
Member

The "Number of unique DNS requests currently in flight" counter doesn't seem critical for 7.0. Moving to future.

but it would be ideal to add some of the other counters

@davidfowl did you have any other counters in mind (aside from those mentioned in #48885 (comment))?

@MihaZupan MihaZupan modified the milestones: 7.0.0, Future Jul 18, 2022
@davidfowl
Copy link
Member

This is something I would like to get done for .NET 8 now that we're moving to metrics. Specifically, a Meter in System.Net that reports the number of DNS resolutions with a host name dimension.

@antonfirsov
Copy link
Member

@davidfowl is DNS the only area you find critical in .NET 8 to define Metrics for, or is there anything from #88383 and #88384 you want us to get done?

@davidfowl
Copy link
Member

davidfowl commented Jul 11, 2023

HTTP counters: #88384 and DNS counters are the most important. The socket and TLS ones I think can wait. Specifically, the counters you cannot write yourselves are the most important for us to add (the connection pooling data that exists inside of SocketHttpHandler's guts).

@antonfirsov
Copy link
Member

That should be all covered by #84978, #88384 is mostly about extras.

@davidfowl
Copy link
Member

Perfect, then I think the DNS one is the next most important one to add that isn't currently tracked for .NET 8.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-System.Net enhancement Product code improvement that does NOT require public API changes/additions
Projects
None yet
Development

No branches or pull requests

8 participants