Skip to content

Fix client monitoring rate limit errors #5189

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

Merged
wemeetagain merged 1 commit into from
Feb 21, 2023
Merged

Fix client monitoring rate limit errors #5189

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
4 changes: 1 addition & 3 deletions docs/usage/client-monitoring.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,9 +40,7 @@ It is also possible to print out the data sent to the remote service by enabling
### Monitoring interval

It is possible to adjust the interval between sending client stats to the remote service by setting the `--monitoring.interval` flag.
It takes an integer value in milliseconds, the default is `62000` which means data is sent approximately once a minute.
The extra 2 seconds are added to avoid rate limit errors when using *beaconcha.in* as they are currently really strict
and minor delays in the previous request can cause the following request to fail.
It takes an integer value in milliseconds, the default is `60000` which means data is sent once a minute.

For example, setting an interval of `300000` would mean the data is only sent every 5 minutes.

Expand Down
4 changes: 1 addition & 3 deletions packages/beacon-node/src/monitoring/options.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,9 +13,7 @@ export type MonitoringOptions = {

export const defaultMonitoringOptions: Required<MonitoringOptions> = {
endpoint: "",
// default interval should be once a minute
// but need to add 2 seconds to avoid rate limit errors
interval: 62_000,
interval: 60_000,
initialDelay: 30_000,
requestTimeout: 10_000,
collectSystemStats: true,
Expand Down
19 changes: 13 additions & 6 deletions packages/beacon-node/src/monitoring/service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ export class MonitoringService {

private status = Status.Stopped;
private initialDelayTimeout?: NodeJS.Timeout;
private monitoringInterval?: NodeJS.Timer;
private monitoringInterval?: NodeJS.Timeout;
private fetchAbortController?: AbortController;
private pendingRequest?: Promise<void>;

Expand Down Expand Up @@ -84,10 +84,7 @@ export class MonitoringService {

this.initialDelayTimeout = setTimeout(async () => {
await this.send();

this.monitoringInterval = setInterval(async () => {
await this.send();
}, interval);
this.nextMonitoringInterval();
}, initialDelay);

this.logger.info("Started monitoring service", {
Expand All @@ -109,7 +106,7 @@ export class MonitoringService {
clearTimeout(this.initialDelayTimeout);
}
if (this.monitoringInterval) {
clearInterval(this.monitoringInterval);
clearTimeout(this.monitoringInterval);
}
if (this.pendingRequest) {
this.fetchAbortController?.abort(FetchAbortReason.Stop);
Expand Down Expand Up @@ -211,6 +208,16 @@ export class MonitoringService {
}
}

private nextMonitoringInterval(): void {
if (this.status === Status.Stopped) return;

@nflaig nflaig Feb 21, 2023 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This check just prevents an edge case which can happen due to a race condition where this.send() has not yet finished but service was stopped, it would invoke this.nextMonitoringInterval() again and keep the interval running even though service is already stopped.

This is probably never relevant if the BN or VC run as standalone services because the whole process will be stopped either way but if it is embedded in another software this might matter.

@nflaig nflaig May 23, 2023

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Just noting here that this change is also quite important if BN or VC are standalone services as setting another timeout would prevent the process from exiting

// ensure next interval only starts after previous request has finished
// else we might send next request too early and run into rate limit errors
this.monitoringInterval = setTimeout(async () => {
await this.send();
this.nextMonitoringInterval();
}, this.options.interval);
}

private parseMonitoringEndpoint(endpoint: string): URL {
if (!endpoint) {
throw new Error("Monitoring endpoint must be provided");
Expand Down
23 changes: 13 additions & 10 deletions packages/beacon-node/test/unit/monitoring/service.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import {expect} from "chai";
import sinon from "sinon";
import sinon, {SinonSpy} from "sinon";
import {ErrorAborted, Logger, TimeoutError} from "@lodestar/utils";
import {RegistryMetricCreator} from "../../../src/index.js";
import {HistogramExtra} from "../../../src/metrics/utils/histogram.js";
Expand Down Expand Up @@ -73,11 +73,12 @@ describe("monitoring / service", () => {
});

it("should set interval to continuously send client stats", async () => {
const setInterval = sandbox.spy(global, "setInterval");
const setTimeout = sandbox.spy(global, "setTimeout");
const interval = 1000;

const service = await startedMonitoringService();
const service = await startedMonitoringService({interval});

expect(setInterval).to.have.been.calledOnce;
expect(setTimeout).to.have.been.calledWithMatch({}, interval);
expect(service["monitoringInterval"]).to.be.an("object");
});

Expand Down Expand Up @@ -116,6 +117,12 @@ describe("monitoring / service", () => {
});

describe("MonitoringService - stop", () => {
let clearTimeout: SinonSpy;

before(() => {
clearTimeout = sandbox.spy(global, "clearTimeout");
});

it("should set the status to stopped", async () => {
const service = await startedMonitoringService();

Expand All @@ -125,23 +132,19 @@ describe("monitoring / service", () => {
});

it("should clear the monitoring interval", async () => {
const clearInterval = sandbox.spy(global, "clearInterval");

const service = await startedMonitoringService();

service.stop();

expect(clearInterval).to.have.been.calledOnceWith(service["monitoringInterval"]);
expect(clearTimeout).to.have.been.calledWith(service["monitoringInterval"]);
});

it("should clear the initial delay timeout", async () => {
const clearTimeout = sandbox.spy(global, "clearTimeout");

const service = await startedMonitoringService({initialDelay: 1000});

service.stop();

expect(clearTimeout).to.have.been.calledOnceWith(service["initialDelayTimeout"]);
expect(clearTimeout).to.have.been.calledWith(service["initialDelayTimeout"]);
});

it("should abort pending requests", async () => {
Expand Down
2 changes: 1 addition & 1 deletion packages/cli/src/cmds/validator/options.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ export const validatorMetricsDefaultOptions = {
};

export const validatorMonitoringDefaultOptions = {
interval: 62_000,
interval: 60_000,
initialDelay: 30_000,
requestTimeout: 10_000,
collectSystemStats: false,
Expand Down
4 changes: 2 additions & 2 deletions packages/cli/test/unit/options/beaconNodeOptions.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ describe("options / beaconNodeOptions", () => {
"metrics.address": "0.0.0.0",

"monitoring.endpoint": "https://beaconcha.in/api/v1/client/metrics?apikey=secretKey&machine=machine1",
"monitoring.interval": 62000,
"monitoring.interval": 60000,
"monitoring.initialDelay": 30000,
"monitoring.requestTimeout": 10000,
"monitoring.collectSystemStats": true,
Expand Down Expand Up @@ -142,7 +142,7 @@ describe("options / beaconNodeOptions", () => {
},
monitoring: {
endpoint: "https://beaconcha.in/api/v1/client/metrics?apikey=secretKey&machine=machine1",
interval: 62000,
interval: 60000,
initialDelay: 30000,
requestTimeout: 10000,
collectSystemStats: true,
Expand Down