Added basic event counters for hosting #10884
Conversation
- Added Request Rate counter (RPS), Total Requests counter, current requests and failed requests. - Counters do nothing until enabled - Added tests for counters and improved tests to use unique event source names - Renamed EventSource to match the convention in the runtime Microsoft.AspNetCore.Hosting
|
|
||
| protected override void OnEventCommand(EventCommandEventArgs command) | ||
| { | ||
| if (command.Command == EventCommand.Enable) |
There was a problem hiding this comment.
From below this line:
// They aren't disabled afterwards...
Do you have more context on why?
There was a problem hiding this comment.
It's racy to disable counters and dispose and re-create them, that would need to be synchronized. The other way to turn it off is to stop pumping the data (that can be done via a different set of arguments), which turns the timer off so allocating the counter itself is fine.
| @@ -136,10 +136,19 @@ public void RequestEnd(HttpContext httpContext, Exception exception, HostingAppl | |||
| StopActivity(httpContext, activity, context.HasDiagnosticListener); | |||
| } | |||
|
|
|||
| if (context.EventLogEnabled && exception != null) | |||
| if (context.EventLogEnabled) | |||
There was a problem hiding this comment.
First time I'm seeing this code, so I know this is already "how it's been" but EventLog != EventSource so this name struck me as odd. I backtracked to where it's initialized and confirmed it was right but the naming seems off.
It's internal so a rename would be safe if you want to be a little more consistent... 🤷♂
There was a problem hiding this comment.
It's public unfortunately. That's why I never renamed it. Can do it later, I think we should make all this stuff internal anyways.
| if (exception != null) | ||
| { | ||
| // Non-inline | ||
| HostingEventSource.Log.UnhandledException(); |
There was a problem hiding this comment.
Is there a reason we don't put exception details in the message?
There was a problem hiding this comment.
- This isn't new code
- You can't pass the exception itself (that isn't supported in EventSource writes), you could pass the text though.
There was a problem hiding this comment.
- This isn't new code
Yes, I am aware.
|
|
||
| protected override void OnEventCommand(EventCommandEventArgs command) | ||
| { | ||
| if (command.Command == EventCommand.Enable) |
There was a problem hiding this comment.
From below this line:
// They aren't disabled afterwards...
Do you have more context on why?
| @@ -35,13 +55,16 @@ public void HostStop() | |||
| [Event(3, Level = EventLevel.Informational)] | |||
| public void RequestStart(string method, string path) | |||
| { | |||
| Interlocked.Increment(ref _totalRequests); | |||
There was a problem hiding this comment.
This is cool stuff. Is it worth investigating using ThreadLocal<int> and summing the .Values in the counter delegate? That would save a bit of time in the hot path by not locking the processor cores. Though I'm probably optimizing a bit prematurely ;)
There was a problem hiding this comment.
Yea I had a version with that though I'll do it after we measure this which I haven't since it involves turning on logging/eventsource which has other overhead. We also still need to interlocked.increment/decrement current requests which also made me go meh.
| @@ -216,5 +263,36 @@ protected override void OnEventWritten(EventWrittenEventArgs eventData) | |||
| } | |||
| } | |||
| } | |||
|
|
|||
| private class CounterListener : EventListener | |||
There was a problem hiding this comment.
This could be useful for testing all counters. We'll have to remember this when adding other counters and promote this to src/Shared (or you could just do it now...)
There was a problem hiding this comment.
There was a problem hiding this comment.
Yep. I have a thread with corefx about how to test these things. As I add more counters I'll move it to shared.
ghost
deleted the
Fixes #9486
How it looks:
cc @shirhatti @DamianEdwards