User/saweb/listen for kestrel events #55
Conversation
| namespace Prometheus.DotNetRuntime.Tests.IntegrationTests | ||
| { | ||
| //[TestFixture] | ||
| //internal class Given_Contention_Events_Are_Enabled_For_Kestrel_Stats |
There was a problem hiding this comment.
I feel like this should have tests but I'm not sure how to write them here.
There was a problem hiding this comment.
The way I've approached integration testing is to invoke some behaviour in the runtime (e.g. allocate some memory, content a lock) and make sure I can observe the relevant change in the metrics.
I'd suggest running a web server and issuing traffic against it, verifying that the expected metrics change.
There was a problem hiding this comment.
@webbsk thanks for submitting this PR! I've had a quick glance over it but due to it's size, it might take a bit of time to give it a proper review. However the functionality you're adding looks very useful, I'll get back to you with a full review when I can.
| @@ -2,13 +2,16 @@ | |||
| <PropertyGroup> | |||
| <TargetFramework>net5.0</TargetFramework> | |||
| <LangVersion>8</LangVersion> | |||
| <DockerDefaultTargetOS>Linux</DockerDefaultTargetOS> | |||
| <DockerfileContext>..\..</DockerfileContext> | |||
There was a problem hiding this comment.
Neat, was not aware of these properties!
| @@ -0,0 +1,1222 @@ | |||
| #!/usr/bin/env bash | |||
There was a problem hiding this comment.
What's the purpose of this script?
| **/secrets.dev.yaml | ||
| **/values.dev.yaml | ||
| LICENSE | ||
| README.md |
| COPY . . | ||
| RUN dotnet publish "examples/AspNetCoreExample" -c Release -o /app | ||
| WORKDIR "/src/examples/AspNetCoreExample" | ||
| RUN dotnet build "AspNetCoreExample.csproj" -c Release -o /app/build |
There was a problem hiding this comment.
There should be no need for an explicit restore or build- publish does all these things implicitly.
| @@ -0,0 +1,13 @@ | |||
| # FROM mcr.microsoft.com/dotnet/core/sdk:3.1.201 AS build | |||
There was a problem hiding this comment.
We should delete this if it's no longer in use. I haven't yet checked out your branch but I still do want to support building the docker image via docker-compose up.
src/prometheus-net.DotNetRuntime/EventListening/Parsers/KestrelEventParser.cs
Show resolved
Hide resolved
|
|
||
| namespace Prometheus.DotNetRuntime.EventListening.Parsers | ||
| { | ||
| public class KestrelEventParser : IEventParser<KestrelEventParser>, KestrelEventParser.Events.Info |
There was a problem hiding this comment.
I can see you went through a fair bit of effort to write this class (the event parsers are by far the most tedious part to write) so firstly, thankyou- you did a good job.
However, it seems that most if not all of the information you're pulling out here could be extracted using event counters. Event counters are definitely prefered for any event that is going to be high-frequency and pretty much required on .NET 3.1 to avoid a nasty CPU leak.
It seems asp.net core 3.1 has the following event counter sources available to it:
OnEventCommand is the method to look at in each class- it will initialize the counters and specifying the name of the counter. Using this name we can subscribe to counters (see RuntimeEventParser as an example).
| @@ -69,9 +69,9 @@ public static class Events | |||
| { | |||
| public interface CountersV3_0 : ICounterEvents | |||
| { | |||
| event Action<MeanCounterValue> ThreadPoolThreadCount; | |||
| event Action<MeanCounterValue> ConnectionCount; | |||
There was a problem hiding this comment.
You've definitely broken the tests with this change- each event source should have it's own class defined. You may be able to group all the ASP.NET counters together as I guess users would commonly want to enable them all simultaneously.
There was a problem hiding this comment.
I've left some feedback for you. There's some fairly large asks in here so feel free to ignore this, I was eventually planning to add support for these counters in and I could definitely build on this MR. If you're still keen, have a look and shoot me any questions you might have.
I think this PR requires work but I'm open to feedback! I am using this in my own work and I would be interested in improvements you suggest.