gather metrics for clouflare vs ovh websockets #16034

schlawg · 2024-09-10T21:24:10Z

set net.socket.test=true in application.conf, restart lila
wait 1-2 weeks
set net.socket.test=false in application.conf, restart lila
wait 1-2 weeks
mongodump -d lichess -c socket_test -o ./for-schlawg
zip for-schlawg and give to me.
db.socket_test.drop()
delete test code

app/views/base/page.scala

ornicar · 2024-09-11T07:24:55Z

I think the PR will be easier to review after master, including #16019, is merged into it

schlawg · 2024-09-11T12:37:55Z

It will also be easier to review once I undo some moving around of stuff in socket.ts while editing. I wasn't thinking about what that would do to the diff.

ornicar · 2024-09-11T12:51:26Z

modules/api/src/main/SocketTestResult.scala

+
+  def isUserInTestBucket(net: NetConfig)(using ctx: Context) =
+    net.socketTest && ctx.pref.usingAltSocket.isEmpty && ctx.userId.exists: userId =>
+      java.security.MessageDigest.getInstance("MD5").digest(userId.value.getBytes).head % 128 == 0


how about ctx.me.exists(_.createdAt.getNano % 128 == 0), that looks cheaper

as long as there is a lila endpoint or dev cli command to check manually (for support). i'm not super attached and saving cycles spent repeatedly xoring numbers seems good, but on the other hand inclusion in the md5 test bucket can be determined with no reliance on prod mongodb.

there's always username.toList.map(_.toInt).sum % 128 and its shorter similar version username.toList.sum % 128

seems fine. i'm not sure we need the avalanche effect of a strong hash. even if the modulo sum of ascii characters results in uneven buckets with weird correlations, it's not a huge deal.

just don't tell niklas or glbert.

~4000/s in prod

ui/common/src/socket.ts

ornicar · 2024-09-11T14:25:29Z

scary changes on socket.ts 👀 Did you try out the affected use cases?

schlawg · 2024-09-11T14:44:26Z

yes, on iOS safari, macOS safari, linux firefox, linux chrome, and android via browserstack samsung galaxy 20ish (?)

from a behavior standpoint, the main difference is we avoid a bunch of scary logs on unexpected connection closures when the browser task queue looks like it was suspended. a false positive (which i never saw) is also no big deal.

this will reduce the number of spurious error reports from suspensions on mobile devices in the collected metrics by roughly 90%. it's not perfect as it's still possible to fail detecting a mobile app suspension when the task queue is suspended from 8.5 to 9.5 seconds after an unacknowledged ping, but this should be good enough.

i played with every type of page lifecycle event imaginable and got the most consistent behavior across all browsers with the task queue monitor approach.

the other functional changes are mostly just to ensure cf and ovh are properly rotated in all scenarios.

schlawg · 2024-09-11T14:44:59Z

that said, maybe niklas should have a look

ui/common/src/socket.ts

schlawg · 2024-09-17T13:26:37Z

ui/common/src/socket.ts

@@ -18,11 +18,10 @@ interface MsgOut extends MsgBase {}
 interface MsgAck extends MsgOut {
  at: number;
 }
-type Send = (t: Tpe, d: Payload, o?: any) => void;


this isn't used anywhere, so the type is described directly where it's used below in Ackable

ui/common/src/socket.ts

schlawg · 2024-09-17T13:27:39Z

ui/common/src/socket.ts


 const isOnline = () => !('onLine' in navigator) || navigator.onLine;

-// versioned events, acks, retries, resync
-export default class StrongSocket {


tie public method typings to the implementation (via extending SocketI which is defined in index.d.ts)

make non-public members private (more as an annotation than access restriction, as they are not available in SocketI anyway)

schlawg · 2024-09-17T13:27:58Z

ui/common/src/socket.ts

-  tryOtherUrl = false;
-  autoReconnect = true;
-  nbConnects = 0;
-  storage: LichessStorage = storage.make(


fix broken this.storage assignment which never actually created 'surl-alt' because socket-alternates is not a valid data attribute, we have been using socket-alts

schlawg · 2024-09-17T13:28:33Z

ui/common/src/socket.ts

  private _sign?: string;
  private resendWhenOpen: [string, any, any][] = [];
-  private baseUrls = (document.body.dataset.socketAlts || document.body.dataset.socketDomains!).split(',');


we always get our socket domains in the same attribute now

schlawg · 2024-09-17T13:31:07Z

ui/common/src/socket.ts


  constructor(
    readonly url: string,
    version: number | false,
    settings: Partial<Settings> = {},
  ) {
+    this.options = {


i moved initialization of the various options, settings, params to the constructor as they are all instance related, whether or not they're hard coded or set by constructor arguments

schlawg · 2024-09-17T13:31:41Z

ui/common/src/socket.ts

@@ -90,49 +93,44 @@ export default class StrongSocket {
        ...(settings.params || {}),
      },
    };
-    const customPingDelay = storedIntProp('socket.ping.interval', 2500)();


customPingDelay was never used

schlawg · 2024-09-17T13:36:19Z

ui/common/src/socket.ts

    this.version = version;
    site.pubsub.on('socket.send', this.send);
    this.connect();
+    this.flushStats();


flushStats is related to the socket test.

so is the 'pagehide' handler. these are discussed below.

schlawg · 2024-09-17T13:40:22Z

ui/common/src/socket.ts

        cl.remove('offline');
        cl.add('online');
-        cl.toggle('reconnected', this.nbConnects > 1);
+        this.onSuccess();


some non-critical stuff shuffled around here, we could actually move all of it to onSuccess, or onOpen if we wanted to maintain uniformity with the websocket event

schlawg · 2024-09-17T13:43:08Z

ui/common/src/socket.ts

  };

-  scheduleConnect = (delay: number): void => {
+  private scheduleConnect = (delay: number = this.options.pongTimeout): void => {


this.pongTimeout is our nominal connect interval, so it's the default value for this call

schlawg · 2024-09-17T13:45:29Z

ui/common/src/socket.ts

-        // if this was set earlier, we've already logged the error
-        this.tryOtherUrl = true;
-        log(`sri ${this.settings.params!.sri} timeout ${delay}ms, trying ${this.baseUrl()}${this.url}`);
-      }


the socket permalogs are noisy and mostly serve to obscure some of the more interesting uncaught exceptions and unhandled promise rejections we get from lichess.org/#debug

schlawg · 2024-09-17T13:46:56Z

ui/common/src/socket.ts

      ws.onerror = ws.onclose = ws.onopen = ws.onmessage = () => {};
      ws.close();
    }
  };

-  onError = (e: unknown): void => {
+  private onError = (e: unknown): void => {
+    if (this.heartbeat.wasSuspended) return;


we ignore errors when they are the expected consequence of task queue suspension

schlawg · 2024-09-17T13:48:19Z

ui/common/src/socket.ts

    site.pubsub.emit('socket.close');
-    if (this.autoReconnect) {
+
+    if (this.heartbeat.wasSuspended) return this.onSuspended();


in order to have valid data, it is critical that we can distinguish task queue suspensions from real network/application errors

schlawg · 2024-09-17T13:49:05Z

ui/common/src/socket.ts

    site.pubsub.emit('socket.close');
-    if (this.autoReconnect) {


this.autoReconnect is actually redundant with this.ws having a value

I'm not sure about that. disconnect sets autoReconnect to false to prevent reconnection. site.reload calls disconnect to prevent websocket activity while the page is reloading. Sometimes the reload is slow and we don't want the websocket to reconnect while it happens.

site.reload calls destroy now. disconnect has been removed from the interface

schlawg · 2024-09-17T13:50:13Z

ui/common/src/socket.ts

      this.debug('Will autoreconnect in ' + this.options.autoReconnectDelay);
      this.scheduleConnect(this.options.autoReconnectDelay);
    }
    if (e.wasClean && e.code < 1002) return;

-    if (isOnline())
-      log(`${site?.sri ? 'sri ' + site.sri : ''} unclean close ${e.code} ${url} ${e.reason}`);
-    this.tryOtherUrl = true;


see above about socket permalogs

schlawg · 2024-09-17T13:51:05Z

ui/common/src/socket.ts

    clearTimeout(this.pingSchedule);
  };

-  onSuccess = (): void => {
-    this.nbConnects++;


this.nbConnects and the related check are redundant with the one time event 'hasConnected'

schlawg · 2024-09-17T13:56:36Z

ui/common/src/socket.ts

+    clearTimeout(this.pingSchedule);
+    clearTimeout(this.connectSchedule);
+    this.storeStats({ event: 'suspend' }).then(this.connect);
+  }


this method is called when a previously loaded page that was suspended becomes active again. we arrive here from close events that are generated for the websocket by the browser on activation.

schlawg · 2024-09-17T14:05:12Z

ui/common/src/socket.ts

@@ -350,14 +352,66 @@ export default class StrongSocket {

  pingInterval = (): number => this.computePingDelay() + this.averageLag;
  getVersion = (): number | false => this.version;
+


these three methods are the bulk of the socket test code. we have three phases.

Track lag stats in memory with updateStats

On orderly connection close (rare) or 'pagehide' (most of our terminations), we call storeStats which dumps the current in-memory stats to local storage. it also calls flushStats but this is only relevant for the orderly close case because that method is async and task & microtask scheduling are no longer available in a pagehide event.

On page loads, we call flushStats which checks local storage for stats from the previous session and sends them to idb (which is async). if the test has ended and there are values in local storage or idb, they are posted in one big batch to the collection endpoint and cleaned up on success.

schlawg · 2024-09-17T14:08:16Z

ui/common/src/timing.ts

@@ -168,3 +168,32 @@ export function debounce<T extends (...args: any) => void>(
      }, wait);
  };
 }
+
+export function browserTaskQueueMonitor(interval = 1000): { wasSuspended: boolean; reset: () => void } {


this is the logic for determining a task queue suspension occurred. false positives are possible in extreme scheduling scenarios, but in the socket case they just result in a rare misclassification of event types in the gathered data.

schlawg · 2024-09-17T14:09:26Z

any other questions about the code in this?

ornicar · 2024-09-18T17:37:09Z

yes, #16034 (comment)

schlawg and others added 4 commits September 10, 2024 16:16

gather metrics for clouflare vs ovh websockets

20c6721

Merge branch 'master' into socket-test

f3e6fba

fix conflict merge

28796f7

move implementations out of view/ and into SocketTest

d7c6831

ornicar reviewed Sep 11, 2024

View reviewed changes

app/views/base/page.scala Show resolved Hide resolved

merge & reorder socket.ts to look like upstream/master

a19db20

ornicar reviewed Sep 11, 2024

View reviewed changes

schlawg and others added 4 commits September 11, 2024 08:33

fix pubsub.past

eddfb63

avoid digesting MD5 twice per page load

1db4f94

~4000/s in prod

streamline runtime

fefdf85

Merge remote-tracking branch 'origin/socket-test' into socket-test

91ba845

ornicar reviewed Sep 11, 2024

View reviewed changes

ui/common/src/socket.ts Show resolved Hide resolved

schlawg requested a review from niklasf September 11, 2024 14:47

schlawg commented Sep 17, 2024

View reviewed changes

ui/common/src/socket.ts Show resolved Hide resolved

schlawg commented Sep 17, 2024

View reviewed changes

ui/common/src/socket.ts Show resolved Hide resolved

schlawg commented Sep 17, 2024

View reviewed changes

murge master

1cfb13c

address disconnect on site.reload timing issue

555cad5

ornicar merged commit 2d7ff82 into lichess-org:master Sep 19, 2024
5 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gather metrics for clouflare vs ovh websockets #16034

gather metrics for clouflare vs ovh websockets #16034

schlawg commented Sep 10, 2024

ornicar commented Sep 11, 2024 •

edited

Loading

schlawg commented Sep 11, 2024

ornicar Sep 11, 2024

schlawg Sep 11, 2024 •

edited

Loading

ornicar Sep 11, 2024 •

edited

Loading

schlawg Sep 11, 2024

schlawg Sep 11, 2024

ornicar commented Sep 11, 2024

schlawg commented Sep 11, 2024

schlawg commented Sep 11, 2024

schlawg Sep 17, 2024

schlawg Sep 17, 2024

schlawg Sep 17, 2024

schlawg Sep 17, 2024 •

edited

Loading

schlawg Sep 17, 2024

schlawg Sep 17, 2024

schlawg Sep 17, 2024

schlawg Sep 17, 2024

schlawg Sep 17, 2024 •

edited

Loading

schlawg Sep 17, 2024

schlawg Sep 17, 2024

schlawg Sep 17, 2024

schlawg Sep 17, 2024

ornicar Sep 18, 2024

schlawg Sep 18, 2024

schlawg Sep 17, 2024

schlawg Sep 17, 2024 •

edited

Loading

schlawg Sep 17, 2024 •

edited

Loading

schlawg Sep 17, 2024

schlawg Sep 17, 2024

schlawg commented Sep 17, 2024

ornicar commented Sep 18, 2024

		@@ -350,14 +352,66 @@ export default class StrongSocket {

		pingInterval = (): number => this.computePingDelay() + this.averageLag;
		getVersion = (): number \| false => this.version;

gather metrics for clouflare vs ovh websockets #16034

gather metrics for clouflare vs ovh websockets #16034

Conversation

schlawg commented Sep 10, 2024

ornicar commented Sep 11, 2024 • edited Loading

schlawg commented Sep 11, 2024

Choose a reason for hiding this comment

schlawg Sep 11, 2024 • edited Loading

Choose a reason for hiding this comment

ornicar Sep 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ornicar commented Sep 11, 2024

schlawg commented Sep 11, 2024

schlawg commented Sep 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schlawg Sep 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schlawg Sep 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schlawg Sep 17, 2024 • edited Loading

Choose a reason for hiding this comment

schlawg Sep 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schlawg commented Sep 17, 2024

ornicar commented Sep 18, 2024

ornicar commented Sep 11, 2024 •

edited

Loading

schlawg Sep 11, 2024 •

edited

Loading

ornicar Sep 11, 2024 •

edited

Loading

schlawg Sep 17, 2024 •

edited

Loading

schlawg Sep 17, 2024 •

edited

Loading

schlawg Sep 17, 2024 •

edited

Loading

schlawg Sep 17, 2024 •

edited

Loading