From c423687aa95c428fc3c781dc92129a8b3074babb Mon Sep 17 00:00:00 2001
From: tuyennhv <tuyen@chainsafe.io>
Date: Sat, 20 Jan 2024 02:31:41 +0700
Subject: [PATCH] feat: log agent version when client is not known (#6314)

* feat: log agent version when client is not known

* chore: add test case for getKnownClientFromAgentVersion
---
 packages/beacon-node/src/network/peers/client.ts      | 11 +++++++++--
 packages/beacon-node/src/network/peers/peerManager.ts |  4 ++--
 packages/beacon-node/src/network/peers/peersData.ts   |  4 ++--
 .../src/network/reqresp/ReqRespBeaconNode.ts          |  3 ++-
 .../test/unit/network/peers/client.test.ts            | 11 ++++++++---
 5 files changed, 23 insertions(+), 10 deletions(-)

diff --git a/packages/beacon-node/src/network/peers/client.ts b/packages/beacon-node/src/network/peers/client.ts
index d75880f07a58..4b84233f6833 100644
--- a/packages/beacon-node/src/network/peers/client.ts
+++ b/packages/beacon-node/src/network/peers/client.ts
@@ -7,7 +7,13 @@ export enum ClientKind {
   Unknown = "Unknown",
 }
 
-export function clientFromAgentVersion(agentVersion: string): ClientKind {
+/**
+ * Get known client from agent version.
+ * If client is not known, don't return ClientKind.Unknown here.
+ * For metrics it'll have fallback logic to use ClientKind.Unknown
+ * For logs, we want to print out agentVersion instead for debugging purposes.
+ */
+export function getKnownClientFromAgentVersion(agentVersion: string): ClientKind | null {
   const slashIndex = agentVersion.indexOf("/");
   const agent = slashIndex >= 0 ? agentVersion.slice(0, slashIndex) : agentVersion;
   const agentLC = agent.toLowerCase();
@@ -16,5 +22,6 @@ export function clientFromAgentVersion(agentVersion: string): ClientKind {
   if (agentLC === "prysm") return ClientKind.Prysm;
   if (agentLC === "nimbus") return ClientKind.Nimbus;
   if (agentLC === "lodestar" || agentLC === "js-libp2p") return ClientKind.Lodestar;
-  return ClientKind.Unknown;
+
+  return null;
 }
diff --git a/packages/beacon-node/src/network/peers/peerManager.ts b/packages/beacon-node/src/network/peers/peerManager.ts
index 5100987566e2..94af8f06ea03 100644
--- a/packages/beacon-node/src/network/peers/peerManager.ts
+++ b/packages/beacon-node/src/network/peers/peerManager.ts
@@ -19,7 +19,7 @@ import {NetworkCoreMetrics} from "../core/metrics.js";
 import {LodestarDiscv5Opts} from "../discv5/types.js";
 import {PeerDiscovery, SubnetDiscvQueryMs} from "./discover.js";
 import {PeersData, PeerData} from "./peersData.js";
-import {clientFromAgentVersion, ClientKind} from "./client.js";
+import {getKnownClientFromAgentVersion, ClientKind} from "./client.js";
 import {
   getConnectedPeerIds,
   hasSomeConnectedPeer,
@@ -615,7 +615,7 @@ export class PeerManager {
         if (agentVersionBytes) {
           const agentVersion = new TextDecoder().decode(agentVersionBytes) || "N/A";
           peerData.agentVersion = agentVersion;
-          peerData.agentClient = clientFromAgentVersion(agentVersion);
+          peerData.agentClient = getKnownClientFromAgentVersion(agentVersion);
         }
       },
       {retries: 3, retryDelay: 1000}
diff --git a/packages/beacon-node/src/network/peers/peersData.ts b/packages/beacon-node/src/network/peers/peersData.ts
index 5487a9a44159..4f96548c73e4 100644
--- a/packages/beacon-node/src/network/peers/peersData.ts
+++ b/packages/beacon-node/src/network/peers/peersData.ts
@@ -38,8 +38,8 @@ export class PeersData {
     return this.connectedPeers.get(peerIdStr)?.agentVersion ?? "NA";
   }
 
-  getPeerKind(peerIdStr: string): ClientKind {
-    return this.connectedPeers.get(peerIdStr)?.agentClient ?? ClientKind.Unknown;
+  getPeerKind(peerIdStr: string): ClientKind | null {
+    return this.connectedPeers.get(peerIdStr)?.agentClient ?? null;
   }
 
   getEncodingPreference(peerIdStr: string): Encoding | null {
diff --git a/packages/beacon-node/src/network/reqresp/ReqRespBeaconNode.ts b/packages/beacon-node/src/network/reqresp/ReqRespBeaconNode.ts
index 962962ab8842..5aea63fb9fe3 100644
--- a/packages/beacon-node/src/network/reqresp/ReqRespBeaconNode.ts
+++ b/packages/beacon-node/src/network/reqresp/ReqRespBeaconNode.ts
@@ -88,7 +88,8 @@ export class ReqRespBeaconNode extends ReqResp {
           metrics?.reqResp.rateLimitErrors.inc({method});
         },
         getPeerLogMetadata(peerId) {
-          return peersData.getPeerKind(peerId);
+          // this logs the whole agent version for unknown client which is good for debugging
+          return peersData.getPeerKind(peerId) ?? peersData.getAgentVersion(peerId);
         },
       }
     );
diff --git a/packages/beacon-node/test/unit/network/peers/client.test.ts b/packages/beacon-node/test/unit/network/peers/client.test.ts
index 75ab4cbfb826..a179d42157cd 100644
--- a/packages/beacon-node/test/unit/network/peers/client.test.ts
+++ b/packages/beacon-node/test/unit/network/peers/client.test.ts
@@ -1,8 +1,8 @@
 import {describe, it, expect} from "vitest";
-import {clientFromAgentVersion, ClientKind} from "../../../../src/network/peers/client.js";
+import {getKnownClientFromAgentVersion, ClientKind} from "../../../../src/network/peers/client.js";
 
 describe("clientFromAgentVersion", () => {
-  const testCases: {name: string; agentVersion: string; client: ClientKind}[] = [
+  const testCases: {name: string; agentVersion: string; client: ClientKind | null}[] = [
     {
       name: "lighthouse",
       agentVersion: "Lighthouse/v2.0.1-fff01b2/x86_64-linux",
@@ -28,11 +28,16 @@ describe("clientFromAgentVersion", () => {
       agentVersion: "js-libp2p/0.32.4",
       client: ClientKind.Lodestar,
     },
+    {
+      name: "unknown client",
+      agentVersion: "strange-client-agent-version",
+      client: null,
+    },
   ];
 
   for (const {name, agentVersion, client} of testCases) {
     it(name, () => {
-      expect(clientFromAgentVersion(agentVersion)).toBe(client);
+      expect(getKnownClientFromAgentVersion(agentVersion)).toBe(client);
     });
   }
 });