Convert SignalR EventSource usage to new pattern #11130
Conversation
BrennanConroy
requested review from
analogrelay,
halter73,
mikaelm12 and
Tratcher
as code owners
|
Nice! The pattern where we check IsEnabled() is broken because you can end up with bad counts if the counters are enabled midway. |
|
So should we move all |
|
/azp run AspNetCore-ci |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
@aspnet/build How does one reset the AspNetCore-ci check without closing and re-opening the PR? |
|
@BrennanConroy what do you mean by "reset" in this context? |
|
There is a red X still even though I have restarted the job. Looks like a similar issue to another PR where the check isn't being updated on a successful build: #10540 |
|
Before you merge, a screenshot with the counters please 😄 |
| private long _currentConnections; | ||
|
|
||
| internal HttpConnectionsEventSource() | ||
| : base("Microsoft-AspNetCore-Http-Connections") |
There was a problem hiding this comment.
Rename to Microsoft.AspNetCore.Http.Connections?
There was a problem hiding this comment.
Should we also normalize the counter names? The hosting ones follow the pattern of "current-requests", and we currently use "CurrentConnections".
There was a problem hiding this comment.
Yes, lets unify the patterns.
There was a problem hiding this comment.
EventSource names have always been Microsoft-Things-Separated-By-Dashes, why would we change that?
Also, we should update the hosting ones since these have already shipped with these names. Plus snake-case is so un-.NET ;)
There was a problem hiding this comment.
Inconsistent AF. This is something we should resolve as part of preview7 (Hosting is updated BTW).
cc @noahfalk
There was a problem hiding this comment.
Well, it's unfortunate that corefx didn't follow our completely undocumented design pattern from 2.2 here 😝. Hard to fix corefx now though. I'm OK with establishing snake-case as the convention if that's the case.
If we do that we have two options:
- Leave the SignalR counters alone. Nobody is "broken" but they look different (though the Display Name is what is generally shown I believe)
- Change the existing SignalR counters to match the new pattern. Anybody depending on the exact name is broken. There weren't great tools for collecting these before 3.0 though so I don't feel too bad breaking that (as long as we Announcements it)
There was a problem hiding this comment.
There's only one option, use the new pattern.
|
|
|
Whats up with the duration counter? Why does it look like that? |
I was going to ask if you knew how to set the display name of a "normal" event counter. |
|
Figured it out, didn't realize
|
|
|
||
| _connectionDuration ??= new EventCounter("connections-duration", this) | ||
| { | ||
| DisplayName = "Average Connection Duration", |
There was a problem hiding this comment.
If we're renaming anyway, this should probably be called "connections-duration-milliseconds".
If we decide that's too verbose, we should still document the units somewhere. The DisplayName should include "milliseconds" or "ms" somewhere.
There was a problem hiding this comment.
There’s a new display units field coming
|
@aspnet/build Please merge :) |
ghost
deleted the
And sneak in a new counter: CurrentConnections