Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add client statistics to the connection spec #308

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Add client statistics to the connection spec #308

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
70f439a
Add client statistics to the connection spec
Jarema Sep 30, 2024
9b236ac
Allow for variance in connects/reconnects
Jarema Oct 1, 2024
bb47e20
Add comments how to handle inconsistencies
Jarema Oct 1, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 30 additions & 3 deletions adr/ADR-40.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
| -------- | ---------- | -------- | ---------------------------- |
| 1 | 2023-10-12 | @Jarema | Initial draft |
| 2 | 2024-6-24 | @aricart | Added protocol error section |
| 3 | 2024-9-30 | @Jarema | Add connection Statistics |

## Summary

Expand Down Expand Up @@ -388,7 +389,7 @@ server. The client will follow its reconnect logic.
`Secure Connection - TLS Required` is sent if the client is trying to connect on
a server that requires TLS.

> [!IMPORTANT]
> [!IMPORTANT]
> The client should have done extensive ServerInfo investigation
> and determined that this would have been a failure when initiating the
> connection.
Expand Down Expand Up @@ -434,7 +435,7 @@ disconnected. Reconnect was greeted with a `Authorization Error`.
`invalid client protocol` sent to the client if the protocol version from the
client doesn't match. Client is disconnected when this error is sent.

> [!NOTE]
> [!NOTE]
> Currently, this is not a concern since presumably, a server will be
> able to deal with protocol version 1 when protocol upgrades.

Expand All @@ -445,7 +446,7 @@ responder, but rejects headers. Client is disconnected when this error is sent.
Current clients hardcode `headers: true`, so this error shouldn't be seen by
clients.

> [!IMPORTANT]
> [!IMPORTANT]
> `headers` connect option shouldn't be exposed by the clients -
> this is a holdover from when clients opted in to `headers`.

Expand Down Expand Up @@ -473,6 +474,32 @@ a command. This is followed by a disconnect.
- `maximum account active connections exceeded` not notified to the client, the
client connecting will be disconnected (seen as a connection refused.)

### Client Statistics
Clients should implement statistics gathered throughout the lifetime of the client (persisted between connections):

#### Bytes In
Should account for everything received from the server, a record of bytes received on the socket.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Jarema - so went looking augment with what is discussed here, but the discussion on the alignment with server is incorrect - Server accounting seems to only include headers/payload for published messages - it doesn't include other protocol messages, subjects, etc. We could add a raw metric for in/out bytes, but right now at least for JavaScript, it lines up with what the server does.

Deno.test("basics - stats", async () => {
  const { ns, nc } = await _setup(connect);

  const cid = nc.info?.client_id || -1;
  if (cid === -1) {
    fail("client_id not found");
  }

  await nc.flush();
  console.log(nc.stats())

  console.log(await ns.connz(cid, "detail"));

  nc.publish("hello", "world");
  await nc.flush();

  console.log(nc.stats())
  console.log(await ns.connz(cid, "detail"));


  nc.subscribe("hello", {callback: () => {}});
  nc.publish("hello", "hi");
  await nc.flush();

  console.log(nc.stats())
  console.log(await ns.connz(cid, "detail"));

  await cleanup(ns, nc);
});
{ inBytes: 0, outBytes: 0, inMsgs: 0, outMsgs: 0 }
{
  server_id: "NBN7DZNKVGIGZBAVMGYTK7RHZZITVELFTCJI5BKOHORHTTO2TOS4OHTL",
  now: "2024-10-15T23:32:44.636996Z",
  num_connections: 1,
  total: 1,
  offset: 0,
  limit: 1,
  connections: [
    {
      cid: 6,
      kind: "Client",
      type: "nats",
      ip: "127.0.0.1",
      port: 59457,
      start: "2024-10-15T18:32:44.633103-05:00",
      last_activity: "2024-10-15T23:32:44.634232Z",
      rtt: "1.129333ms",
      uptime: "0s",
      idle: "0s",
      pending_bytes: 0,
      in_msgs: 0,
      out_msgs: 0,
      in_bytes: 0,
      out_bytes: 0,
      subscriptions: 0,
      lang: "nats.deno",
      version: "3.0.0-6"
    }
  ]
}
{ inBytes: 0, outBytes: 5, inMsgs: 0, outMsgs: 1 }
{
  server_id: "NBN7DZNKVGIGZBAVMGYTK7RHZZITVELFTCJI5BKOHORHTTO2TOS4OHTL",
  now: "2024-10-15T23:32:44.638501Z",
  num_connections: 1,
  total: 1,
  offset: 0,
  limit: 1,
  connections: [
    {
      cid: 6,
      kind: "Client",
      type: "nats",
      ip: "127.0.0.1",
      port: 59457,
      start: "2024-10-15T18:32:44.633103-05:00",
      last_activity: "2024-10-15T18:32:44.638146-05:00",
      rtt: "1.129333ms",
      uptime: "0s",
      idle: "0s",
      pending_bytes: 0,
      in_msgs: 1,
      out_msgs: 0,
      in_bytes: 5,
      out_bytes: 0,
      subscriptions: 0,
      lang: "nats.deno",
      version: "3.0.0-6"
    }
  ]
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok. I probably made a mistake checking the server code.
Though I still agree with your point that we should count everything in and out.
We for sure can have more stats - current one and raw.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that would be the correct thing to do. The other nuance is we should reset on reconnect the current counters this way the reporting from the client will always match the current session and the server. We can add a total_in/out which accounts for all the in/out but these are always all session - Not sure what the benefit is to say that a current client over all its reconnects has sent XX amount of data, so preserving that session data is possibly not useful.


#### Bytes Out
Should account for everything sent to the server, a record of bytes sent on the socket.

#### Messages In
Every message (`MSG` and `HMSG`) received from the server.

#### Messages out
Every message (`PUB` and `HPUB`) sent to the server.

#### Connects / Reconnects
Total number of connections (first connection and any successful reconnection) or reconnections made by client.
We allow both variants here, as languages have some variance in how they handle connections around option `retry_on_failed_connect`.

#### Handling inconsistencies across clients

As `Statistics` has been added before we had a concept of ADRs and parity, each client differs in what and how it counts.
To achieve parity without introducing breaking changes, new fields should use appropriate names.
For example, Go could add `TotalBytesIn` and `TotalBytesOut`.
Old fields should be kept with added comments describing what they are counting.

### Security Considerations

Discuss any additional security considerations pertaining to the TLS
Expand Down
Loading