Elected summarizer logs

[arinwt]

Fixes #5617 by adding telemetry for when a single elected client fails to summarize for long enough. This counter resets every time the elected summarizer changes, so it isn't tracking per document as a whole like the one in ContainerRuntime is (those events are "SummaryStatus:Behind" and "SummaryStatus:CaughtUp").

This required different ordered client tracking. Used a specialized two-way linked list to track ordered clients for quick calculation of elected client- including increment and reset, plus a map for quick removal. The adding should also be constant time unless somehow they come out of order. This replaces the QuorumHeap previously used. Added OrderedClients in a new file: summaryManagerUtils.ts with tests around it. Also put the Throttler here.

Also change SummaryCollection to use events instead of op actions.

[anthony-murphy]

@tylerbutler any idea why this keeps flip flopping? seen in it my changes as well

[tylerbutler]

Yeah I've noticed it as well. No idea why yet. I'll open an issue. #6158

[anthony-murphy]

do we want to track acks, or just ops? I've gone back and forth a bit on this. if one client's ops fail to ack, do we think another client's will?

[arinwt]

Acks is more important, but both can be valuable: each summarizer client tracks both for itself- attempt vs success. And no, it's not true that summary attempts that are nacked by one client imply that they will definitely be nacked by another client, especially if the internal state tracking of that summarizer client is busted. Though, in most cases simply restarting the client would be sufficient.

[anthony-murphy]

not normal being the initiall population of the list?

[arinwt]

Theoretically I guess, I'm not really sure what case allows them to come in unordered, since they are serialized as an array and already sorted naturally. I did not want to assume that serialization/deserialization rule just yet though.

[anthony-murphy]

i don't know what current is yet, but here it looks the be the first last client. which is strange

[arinwt]

I revamped it a little, so instead of using overly abstract terms like "current client", "last client", and "non-summarizer clients", it is now "elected client", "youngest client", and "eligible clients". So basically this thing tracks ordered clients for purposes of election only. Eligible is the abstraction over interactive, but exists in case we wanted that to mean something else.

[anthony-murphy]

could this be made simpler if this always just returne root.next, and increment was changed to remove root.next. what's the use of keep the invalid clients around

[arinwt]

We have the reset functionality, which we will likely need when all clients have been marked as bad summarizers we need to start over and probably just blow up or delay cycling through them again or something.

[msfluid-bot]

■ @fluidframework/base-host: No change

Metric Name	Baseline Size	Compare Size	Size Diff
main.js	177.17 KB	177.17 KB	■ No change
Total Size	177.17 KB	177.17 KB	■ No change

⯅ @fluid-example/bundle-size-tests: +2.13 KB

Metric Name	Baseline Size	Compare Size	Size Diff
container.js	205.99 KB	208.12 KB	⯅ +2.13 KB
map.js	50.1 KB	50.1 KB	■ No change
matrix.js	146.09 KB	146.09 KB	■ No change
odspDriver.js	196.56 KB	196.56 KB	■ No change
odspPrefetchSnapshot.js	27.39 KB	27.39 KB	■ No change
sharedString.js	162.44 KB	162.44 KB	■ No change
Total Size	788.58 KB	790.7 KB	⯅ +2.13 KB

---

Baseline commit: ecbf0d8

Generated by 🚫 dangerJS against e829909

[anthony-murphy]

this.electedClientId

can we get rid of the member this.electedClientId, and just make it a getter that returns this.orderedClients.getElectedClient()?.clientId. I believe this method will only be called when the summarizer has changed. not needed. i just like to reduce state when we can

[arinwt]

Actually no, I think we should keep it for now. Agree with the sentiment of reducing state, but right now it does prevent excess "summarizer" event emitting. We call this function every time any change happens.

[anthony-murphy]

should this ever return root, seems like it can now. i think in that case it should return undefined

[arinwt]

It shouldn't and cannot right now. It only ever gets set to youngerClient or newClient. And no client has the root as its youngerClient.

1. Add client will set it to the added client in some cases (new)
2. Remove client will set it to the younger client (next)
3. Increment will set it to the younger client (next)
4. Reset will set it to the root's younger client (next)

[anthony-murphy]

why not just a normal function?

[arinwt]

I just did it because in the constructor I attach addClient and removeClient as event handlers, and I want to make sure I encapsulate the this correctly without having to wrap them in lambdas. Equivalent:

class A {
    public readonly x: string = "x";

    constructor(caller: (f: () => void) => void) {
        caller(this.asFunction);
        caller(this.asProperty);
    }

    public asFunction(): void {
        console.log(`${this?.x ?? 'u'} as function`);
    }

    public readonly asProperty = () => {
        console.log(`${this?.x ?? 'u'} as function`);
    }
}

new A((f) => f());

Prints "u as function" and "x as property" showing that this is correctly captured since it's a property on each instance vs. a function on the prototype or something something JavaScript something something...

[anthony-murphy]

🕐

[anthony-murphy]