Add ability to observe the number of connections halibut opens for polling connections - #717
Add ability to observe the number of connections halibut opens for polling connections#717pawelpabich wants to merge 6 commits into
Conversation
…ch polling subscription
| await IdentifyAsServerAsync(identity, cancellationToken); | ||
| await ProcessClientRequestsAsync(incomingRequestProcessor, cancellationToken); | ||
| break; | ||
| case RemoteIdentityType.Subscriber: |
There was a problem hiding this comment.
I've moved the connection limit enforcement closer to the code that it applies to.
- There is no need to perform work for other types of remote identities
- We were executing
IdentifyAsServerAsynceven for the default switch case, which results in an exception. This shows how easy it is to make a mistake if code that is case-specific is applied to all cases.
There was a problem hiding this comment.
How was the issue found?
| /// <param name="currentCount"> | ||
| /// The number of active TCP connections for this subscriptionId immediately after the change | ||
| /// </param> | ||
| public void ConnectionsCountChangedFor(Uri subscriptionId, int previousCount, int currentCount); |
There was a problem hiding this comment.
I considrerd a pair of ConnectionOpendFor and ConnectionClosedFor methods but then we would keep yet another copy of <SubscriptionId, int> in the metric producer in the server. This might matter when the Server needs to deal with 20k of tentacles. Thoughts?
There was a problem hiding this comment.
Make it simpler for the caller is probably the best approach.
There was a problem hiding this comment.
Looking at how this is used, what we have now is fine.
| readonly HalibutTimeoutsAndLimits timeoutsAndLimits; | ||
| readonly IConnectionsObserver connectionsObserver; | ||
|
|
||
| Dictionary<Uri, StrongBox<int>> activeConnectionCountPerSubscriptionId = new(); |
There was a problem hiding this comment.
This looks like the information you want.
Maybe have a method that will give you back a copy of this dictionary upon request.
There was a problem hiding this comment.
That would create a tighter coupling between these two.
There was a problem hiding this comment.
I think what you are doing with buckets is faster over processing the entire dict each time we want metrics so this is fine.
As for coupling we already have some form, I think the form that works the best for the telemetry we want to collect makes sense. I guess that comes with a "I would be comfortable changing the contract for telemetry callbacks if we had"
|
|
||
| count.Value++; | ||
|
|
||
| connectionsObserver.ConnectionsCountChangedFor(subscriptionId, previousCount, count.Value); |
There was a problem hiding this comment.
I guess this means we need to be super careful about what ConnectionsCountChangedFor does since slow code here will impact ALL connecting tentacles one after the other.
There was a problem hiding this comment.
Yes, the same applies to other methods of the observers. The only difference is that previously only a single connection would be blocked whereas here all new connections would be blocked. Witout the lock, we would get inconsitent results.
There was a problem hiding this comment.
Actually with your implementation, the call to ConnectionsCountChangedFor does not need to be within the lock provided that the values we would pass in are immutable at this point.
I think I would suggest preserving the values and calling ConnectionsCountChangedFor outside the lock, so as to prevent coupling where someone depends on being within the lock.
I am happy for the PR to go in with the lock around that call but would prefer it not be around the call. We can always move the call to be within the lock later but going the other way is a little more tricky.
There was a problem hiding this comment.
Without lock we will be exposed to suprising short term values. E.g.
(1, 2) might be processed before (0, 1) which will result in (-1, 1) -> (0, 1) values in the bucket. (-1, 1) should not really last for long as the connections are rather long lived. I will move it.
|
Tests added. |
pawelpabich
changed the title
2 participants
Related server PR https://github.com/OctopusDeploy/OctopusDeploy/pull/46535