Add System.Net.NameResolution metrics #68
Conversation
|
Hey, thanks for the submission! I'm on holiday from now until the end of May but I'll try and take a look at this over the next couple of weeks. |
djluck
left a comment
djluck
left a comment
There was a problem hiding this comment.
Thank you so much for this quality submission. I have one blocking comment around the name and type of metric that we are using to measure average duration lookup but that's it, all other comments are just clarifications.
| while (!_collector.EventListeners.All(x => x.StartedReceivingEvents)) | ||
|
|
||
| Console.Write("Waiting for event listeners to be active.. "); | ||
| if (!SpinWait.SpinUntil(() => |
There was a problem hiding this comment.
Is there a benefit in switching from a blocking wait to a spin wait?
| lastLookups = e.Mean; | ||
| }; | ||
|
|
||
| DnsLookupDuration = metrics.CreateHistogram("dotnet_dns_lookup_duration_avg_seconds", |
There was a problem hiding this comment.
I have some issues with taking the average and representing it as a histogram:
- The single sample produced from the event parser every
nseconds will see invalid values set for the_countand_sumvalues. - The name includes
_avg_which is confusing for a histogram
I would instead look at creating a gauge to capture the average lookup duration. A histogram would only be viable if we were listening to the individual DNS lookup events and we could track the individual times of each resolution.
| .Where(s => allMetrics.SourceToMetrics[s].Count > 0) | ||
| .ToList(); | ||
|
|
||
| if (nonEmptySources.Count == 0) |
There was a problem hiding this comment.
It was my original intention to not render anything for runtime versions where no metrics could be produced. Do you find this alternative easier to understand?
2 participants
Adds new metrics on .NET 5+:
dotnet_dns_lookups_total: The number of DNS lookups requested since the process starteddotnet_dns_lookup_duration_avg_seconds: The average time taken for a DNS lookup