Skip to content

Commit 0c9ce4e

Browse files
ctw-joao-luisluiz-bezerra-ctw-bmwgroup-com_QCOM
ctw-joao-luis
and
luiz-bezerra-ctw-bmwgroup-com_QCOM
authored
Fix alert spam for high message frequency on ws (#962)
## User-Facing Changes Fixed an alert spam issue in the WebSocket player where high message frequency warnings were triggered repeatedly. The alert now fires only once per connection session. This fixes #951 ## Description The `FoxgloveWebSocketPlayer` was not tracking whether a high-frequency topic had already been detected, causing the alert to be re-evaluated and re-added on every incoming message. This PR fixes that and also refactors the shared high-frequency detection utility to be cleaner and usable by both players. Changes: - Added `ishighFrequencyMessage` class field (reset on reconnection in the "open" handler) to ensure `isTopicHighFrequency` is only called until a high-frequency topic is detected. The alert is now added inline, removing the need for the `checkForHighFrequencyTopics` wrapper. - `isTopicHighFrequency`: removed unused `alerts`, `connectionType`, `startTime`, and `endTime` parameters. The function now takes only `{ topicStats, topic, duration }`, making it usable directly from both `FoxgloveWebSocketPlayer` and `IterablePlayer` without extra coupling; - `helpers.ts`: Deleted `checkForHighFrequencyTopics` wrapper function; - `types.ts`: Deleted `CheckForHighFrequencyTopics` type and its orphaned imports; - `IterablePlayer`: Updated call site to the new `isTopicHighFrequency` signature; - Tests updated across `isTopicHighFrequency.test.ts` and `helpers.test.ts`. ## How to test it Created this repo that can be used to test it: https://github.com/ctw-joao-luis/lichtblick-high-frequency-ws ## Checklist - [ ] The web version was tested and it is running ok - [ ] The desktop version was tested and it is running ok - [ ] This change is covered by unit tests - [ ] Files constants.ts, types.ts and *.style.ts have been checked and relevant code snippets have been relocated --------- Signed-off-by: Bezerra Luiz, (Luiz.Bezerra@ctw.bmwgroup.com) <198787532+luiz-bezerra-ctw-bmwgroup-com_QCOM@users.noreply.github.com> Co-authored-by: Bezerra Luiz, (Luiz.Bezerra@ctw.bmwgroup.com) <198787532+luiz-bezerra-ctw-bmwgroup-com_QCOM@users.noreply.github.com>
1 parent 2f00248 commit 0c9ce4e

File tree

10 files changed

+113
-199
lines changed

10 files changed

+113
-199
lines changed

‎packages/suite-base/src/players/FoxgloveWebSocketPlayer/helpers.test.ts‎

Lines changed: 0 additions & 101 deletions
Original file line numberDiff line numberDiff line change
@@ -4,19 +4,11 @@
44
import { StatusLevel } from "@foxglove/ws-protocol";
55

66
import {
7-
checkForHighFrequencyTopics,
87
dataTypeToFullName,
98
statusLevelToAlertSeverity,
109
} from "@lichtblick/suite-base/players/FoxgloveWebSocketPlayer/helpers";
11-
import { CheckForHighFrequencyTopics } from "@lichtblick/suite-base/players/FoxgloveWebSocketPlayer/types";
12-
import PlayerAlertManager from "@lichtblick/suite-base/players/PlayerAlertManager";
13-
import { isTopicHighFrequency } from "@lichtblick/suite-base/players/utils/isTopicHighFrequency";
14-
import PlayerBuilder from "@lichtblick/suite-base/testing/builders/PlayerBuilder";
15-
import RosTimeBuilder from "@lichtblick/suite-base/testing/builders/RosTimeBuilder";
1610
import { BasicBuilder } from "@lichtblick/test-builders";
1711

18-
jest.mock("@lichtblick/suite-base/players/utils/isTopicHighFrequency");
19-
2012
describe("dataTypeToFullName", () => {
2113
it("should convert dataType to include /msg/ on it", () => {
2214
const message = "unit/test";
@@ -46,96 +38,3 @@ describe("statusLevelToProblemSeverity", () => {
4638
expect(statusLevelToAlertSeverity(level)).toBe(result);
4739
});
4840
});
49-
50-
describe("checkForHighFrequencyTopics", () => {
51-
let mockIsTopicHighFrequency: jest.MockedFunction<typeof isTopicHighFrequency>;
52-
53-
function buildParams(
54-
override: Partial<CheckForHighFrequencyTopics> = {},
55-
): CheckForHighFrequencyTopics {
56-
return {
57-
endTime: RosTimeBuilder.time(),
58-
startTime: RosTimeBuilder.time(),
59-
topics: PlayerBuilder.topics(),
60-
topicStats: BasicBuilder.genericMap(PlayerBuilder.topicStats),
61-
alerts: new PlayerAlertManager(),
62-
...override,
63-
};
64-
}
65-
66-
beforeEach(() => {
67-
jest.clearAllMocks();
68-
mockIsTopicHighFrequency = jest.mocked(isTopicHighFrequency);
69-
});
70-
71-
describe("Given undefined parameters", () => {
72-
it("should return early when endTime is undefined", () => {
73-
// Given
74-
const input = buildParams({ endTime: undefined });
75-
76-
// When
77-
checkForHighFrequencyTopics(input);
78-
79-
// Then
80-
expect(mockIsTopicHighFrequency).not.toHaveBeenCalled();
81-
});
82-
83-
it("should return early when startTime is undefined", () => {
84-
// Given
85-
const input = buildParams({ startTime: undefined });
86-
87-
// When
88-
checkForHighFrequencyTopics(input);
89-
90-
// Then
91-
expect(mockIsTopicHighFrequency).not.toHaveBeenCalled();
92-
});
93-
94-
it("should return early when topics is undefined", () => {
95-
// Given
96-
const input = buildParams({ topics: undefined });
97-
98-
// When
99-
checkForHighFrequencyTopics(input);
100-
101-
// Then
102-
expect(mockIsTopicHighFrequency).not.toHaveBeenCalled();
103-
});
104-
105-
it("should return early when topics array is empty", () => {
106-
// Given
107-
const input = buildParams({ topics: [] });
108-
109-
// When
110-
checkForHighFrequencyTopics(input);
111-
112-
// Then
113-
expect(mockIsTopicHighFrequency).not.toHaveBeenCalled();
114-
});
115-
});
116-
117-
describe("Given valid parameters", () => {
118-
it("should return early when first topic is high frequency", () => {
119-
// Given
120-
const input = buildParams({
121-
endTime: RosTimeBuilder.time({ sec: 60, nsec: 0 }),
122-
startTime: RosTimeBuilder.time({ sec: 10, nsec: 0 }),
123-
});
124-
125-
mockIsTopicHighFrequency.mockReturnValueOnce(true);
126-
127-
// When
128-
checkForHighFrequencyTopics(input);
129-
130-
// Then
131-
expect(mockIsTopicHighFrequency).toHaveBeenCalledTimes(1);
132-
expect(mockIsTopicHighFrequency).toHaveBeenCalledWith(
133-
input.topicStats,
134-
input.topics![0]!.name,
135-
{ sec: 50, nsec: 0 }, // duration should be subtractTimes(endTime, startTime)
136-
input.topics![0]!.schemaName,
137-
input.alerts,
138-
);
139-
});
140-
});
141-
});

‎packages/suite-base/src/players/FoxgloveWebSocketPlayer/helpers.ts‎

Lines changed: 0 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -3,10 +3,7 @@
33

44
import { StatusLevel } from "@foxglove/ws-protocol";
55

6-
import { CheckForHighFrequencyTopics } from "@lichtblick/suite-base/players/FoxgloveWebSocketPlayer/types";
7-
import { subtractTimes } from "@lichtblick/suite-base/players/UserScriptPlayer/transformerWorker/typescript/userUtils/time";
86
import { PlayerAlert } from "@lichtblick/suite-base/players/types";
9-
import { isTopicHighFrequency } from "@lichtblick/suite-base/players/utils/isTopicHighFrequency";
107

118
export function dataTypeToFullName(dataType: string): string {
129
const parts = dataType.split("/");
@@ -25,30 +22,3 @@ export function statusLevelToAlertSeverity(level: StatusLevel): PlayerAlert["sev
2522
return "error";
2623
}
2724
}
28-
29-
export function checkForHighFrequencyTopics({
30-
alerts,
31-
endTime,
32-
startTime,
33-
topics,
34-
topicStats,
35-
}: CheckForHighFrequencyTopics): void {
36-
if (!endTime || !startTime || !topics || topics.length === 0) {
37-
return;
38-
}
39-
40-
const duration = subtractTimes(endTime, startTime);
41-
42-
for (const topic of topics) {
43-
const highFrequency = isTopicHighFrequency(
44-
topicStats,
45-
topic.name,
46-
duration,
47-
topic.schemaName,
48-
alerts,
49-
);
50-
if (highFrequency) {
51-
return;
52-
}
53-
}
54-
}

‎packages/suite-base/src/players/FoxgloveWebSocketPlayer/index.ts‎

Lines changed: 29 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,14 @@ import { MessageDefinition, isMsgDefEqual } from "@lichtblick/message-definition
2929
import CommonRosTypes from "@lichtblick/rosmsg-msgs-common";
3030
import { MessageWriter as Ros1MessageWriter } from "@lichtblick/rosmsg-serialization";
3131
import { MessageWriter as Ros2MessageWriter } from "@lichtblick/rosmsg2-serialization";
32-
import { fromMillis, fromNanoSec, isGreaterThan, isLessThan, Time } from "@lichtblick/rostime";
32+
import {
33+
fromMillis,
34+
fromNanoSec,
35+
isGreaterThan,
36+
isLessThan,
37+
subtract,
38+
Time,
39+
} from "@lichtblick/rostime";
3340
import { ParameterValue } from "@lichtblick/suite";
3441
import { Asset } from "@lichtblick/suite-base/components/PanelExtensionAdapter";
3542
import PlayerAlertManager from "@lichtblick/suite-base/players/PlayerAlertManager";
@@ -48,6 +55,8 @@ import {
4855
Topic,
4956
TopicStats,
5057
} from "@lichtblick/suite-base/players/types";
58+
import { HIGH_FREQUENCY_ALERT } from "@lichtblick/suite-base/players/utils/constants";
59+
import { isTopicHighFrequency } from "@lichtblick/suite-base/players/utils/isTopicHighFrequency";
5160
import rosDatatypesToMessageDefinition from "@lichtblick/suite-base/util/rosDatatypesToMessageDefinition";
5261

5362
import { JsonMessageWriter } from "./JsonMessageWriter";
@@ -63,11 +72,7 @@ import {
6372
SUPPORTED_SERVICE_ENCODINGS,
6473
ZERO_TIME,
6574
} from "./constants";
66-
import {
67-
checkForHighFrequencyTopics,
68-
dataTypeToFullName,
69-
statusLevelToAlertSeverity,
70-
} from "./helpers";
75+
import { dataTypeToFullName, statusLevelToAlertSeverity } from "./helpers";
7176
import {
7277
MessageWriter,
7378
MessageDefinitionMap,
@@ -142,6 +147,7 @@ export default class FoxgloveWebSocketPlayer implements Player {
142147
#fetchedAssets = new Map<string, Promise<Asset>>();
143148
#parameterTypeByName = new Map<string, Parameter["type"]>();
144149
#messageSizeEstimateByTopic: Record<string, number> = {};
150+
#ishighFrequencyMessage = false;
145151

146152
public constructor({
147153
url,
@@ -217,6 +223,7 @@ export default class FoxgloveWebSocketPlayer implements Player {
217223
this.#advertisedServices = undefined;
218224
this.#datatypes = new Map();
219225
this.#parameters = new Map();
226+
this.#ishighFrequencyMessage = false;
220227
});
221228

222229
this.#client.on("error", (err) => {
@@ -568,13 +575,22 @@ export default class FoxgloveWebSocketPlayer implements Player {
568575
stats.numMessages++;
569576
this.#topicsStats = topicStats;
570577

571-
checkForHighFrequencyTopics({
572-
alerts: this.#alerts,
573-
endTime: this.#endTime,
574-
startTime: this.#startTime,
575-
topics: this.#topics,
576-
topicStats: this.#topicsStats,
577-
});
578+
if (!this.#ishighFrequencyMessage) {
579+
const duration =
580+
this.#startTime && this.#endTime ? subtract(this.#endTime, this.#startTime) : undefined;
581+
this.#ishighFrequencyMessage = isTopicHighFrequency({
582+
topicStats: this.#topicsStats,
583+
topic: { name: topic, schemaName: chanInfo.channel.schemaName },
584+
duration,
585+
});
586+
if (this.#ishighFrequencyMessage) {
587+
this.#alerts.addAlert(HIGH_FREQUENCY_ALERT.id, {
588+
severity: HIGH_FREQUENCY_ALERT.severity,
589+
message: HIGH_FREQUENCY_ALERT.message,
590+
error: new Error(HIGH_FREQUENCY_ALERT.errorMessage),
591+
});
592+
}
593+
}
578594
} catch (error) {
579595
this.#alerts.addAlert(`message:${chanInfo.channel.topic}`, {
580596
severity: "error",

‎packages/suite-base/src/players/FoxgloveWebSocketPlayer/types.ts‎

Lines changed: 0 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -11,9 +11,6 @@ import { ParsedChannel } from "@lichtblick/mcap-support";
1111
import { MessageDefinition } from "@lichtblick/message-definition";
1212
import { MessageWriter as Ros1MessageWriter } from "@lichtblick/rosmsg-serialization";
1313
import { MessageWriter as Ros2MessageWriter } from "@lichtblick/rosmsg2-serialization";
14-
import { Time } from "@lichtblick/rostime";
15-
import PlayerAlertManager from "@lichtblick/suite-base/players/PlayerAlertManager";
16-
import { Topic, TopicStats } from "@lichtblick/suite-base/players/types";
1714

1815
export type ResolvedChannel = {
1916
channel: Channel;
@@ -40,11 +37,3 @@ export type ToWorkerMessage =
4037
export interface MessageWriter {
4138
writeMessage(message: unknown): Uint8Array;
4239
}
43-
44-
export type CheckForHighFrequencyTopics = {
45-
alerts: PlayerAlertManager;
46-
endTime: Time | undefined;
47-
startTime: Time | undefined;
48-
topics: Topic[] | undefined;
49-
topicStats: Map<string, TopicStats>;
50-
};

‎packages/suite-base/src/players/IterablePlayer/IterablePlayer.test.ts‎

Lines changed: 11 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ import { signal } from "@lichtblick/den/async";
1212
import { fromSec } from "@lichtblick/rostime";
1313
import { PLAYER_CAPABILITIES } from "@lichtblick/suite-base/players/constants";
1414
import { MessageEvent, PlayerPresence, PlayerState } from "@lichtblick/suite-base/players/types";
15+
import { HIGH_FREQUENCY_ALERT } from "@lichtblick/suite-base/players/utils/constants";
1516
import * as highFrequencyUtils from "@lichtblick/suite-base/players/utils/isTopicHighFrequency";
1617
import { mockTopicSelection } from "@lichtblick/suite-base/test/mocks/mockTopicSelection";
1718

@@ -705,8 +706,8 @@ describe("IterablePlayer", () => {
705706
const playerStates = await store.done;
706707
expect(_.last(playerStates)!.alerts).toEqual([
707708
{
708-
severity: "warn",
709-
message: "High frequency topics detected",
709+
severity: HIGH_FREQUENCY_ALERT.severity,
710+
message: HIGH_FREQUENCY_ALERT.message,
710711
error: expect.any(Error),
711712
},
712713
]);
@@ -769,9 +770,16 @@ describe("IterablePlayer", () => {
769770
await store.add(state);
770771
});
771772

772-
await store.done;
773+
const playerStates = await store.done;
773774

774775
expect(isTopicHighFrequencySpy).toHaveBeenCalledTimes(1);
776+
expect(_.last(playerStates)!.alerts).toEqual([
777+
{
778+
severity: HIGH_FREQUENCY_ALERT.severity,
779+
message: HIGH_FREQUENCY_ALERT.message,
780+
error: expect.any(Error),
781+
},
782+
]);
775783

776784
player.close();
777785
await player.isClosed;

‎packages/suite-base/src/players/IterablePlayer/IterablePlayer.ts‎

Lines changed: 14 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -44,6 +44,7 @@ import {
4444
TopicSelection,
4545
TopicStats,
4646
} from "@lichtblick/suite-base/players/types";
47+
import { HIGH_FREQUENCY_ALERT } from "@lichtblick/suite-base/players/utils/constants";
4748
import { isTopicHighFrequency } from "@lichtblick/suite-base/players/utils/isTopicHighFrequency";
4849
import { RosDatatypes } from "@lichtblick/suite-base/types/RosDatatypes";
4950
import delay from "@lichtblick/suite-base/util/delay";
@@ -559,7 +560,7 @@ export class IterablePlayer implements Player {
559560
const uniqueTopics = new Map<string, Topic>();
560561
const duration = subtractTimes(this.#end, this.#start);
561562
this.#providerTopicStats = topicStats;
562-
let highFrequencyTopicFound = false;
563+
let hasHighFrequencyTopic = false;
563564

564565
for (const topic of topics) {
565566
const existingTopic = uniqueTopics.get(topic.name);
@@ -573,14 +574,20 @@ export class IterablePlayer implements Player {
573574
}
574575
uniqueTopics.set(topic.name, topic);
575576

576-
if (!highFrequencyTopicFound) {
577-
highFrequencyTopicFound = isTopicHighFrequency(
577+
if (!hasHighFrequencyTopic) {
578+
hasHighFrequencyTopic = isTopicHighFrequency({
578579
topicStats,
579-
topic.name,
580+
topic,
580581
duration,
581-
topic.schemaName,
582-
this.#alertManager,
583-
);
582+
});
583+
584+
if (hasHighFrequencyTopic) {
585+
this.#alertManager.addAlert(HIGH_FREQUENCY_ALERT.id, {
586+
severity: HIGH_FREQUENCY_ALERT.severity,
587+
message: HIGH_FREQUENCY_ALERT.message,
588+
error: new Error(HIGH_FREQUENCY_ALERT.errorMessage),
589+
});
590+
}
584591
}
585592
}
586593

‎packages/suite-base/src/players/utils/constants.ts‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,14 @@
33

44
export const FREQUENCY_LIMIT = 60;
55

6+
export const HIGH_FREQUENCY_ALERT = {
7+
id: "high-frequency",
8+
severity: "warn" as const,
9+
message: "High frequency topics detected",
10+
errorMessage:
11+
"The current data source has one or more topics with message frequency higher than 60Hz, which may impact performance and application memory.",
12+
};
13+
614
export const LOG_SCHEMAS = new Set([
715
"rosgraph_msgs/Log", // ROS 1
816
"rosgraph_msgs/msg/Log", // ROS 1 (alternative format)

0 commit comments

Comments
 (0)