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.

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

Improve performance when logging is disabled #23893

Merged
merged 29 commits into from
Sep 12, 2024
Merged
Show file tree
Hide file tree
Changes from 16 commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
0a06d37
Improve log performance
tomer-w Sep 7, 2024
6765245
Merge branch 'master' of https://github.com/tomer-w/zigbee2mqtt
tomer-w Sep 7, 2024
83ba5a2
Bug fix
tomer-w Sep 7, 2024
a00f522
Revert sample configuration
tomer-w Sep 7, 2024
cac4b34
Align package versions
tomer-w Sep 7, 2024
16f550c
Merge branch 'dev' into master
tomer-w Sep 7, 2024
1632163
Revert dependecies version to allow testing
tomer-w Sep 8, 2024
80f0637
Merge branch 'master' of https://github.com/tomer-w/zigbee2mqtt
tomer-w Sep 8, 2024
ff732a1
Update package-lock.json
tomer-w Sep 8, 2024
db8fc5f
Fix prettier issue
tomer-w Sep 8, 2024
97e56ba
Coverage to 100%
tomer-w Sep 8, 2024
6b4be32
Fix CR comments
tomer-w Sep 9, 2024
7842842
remove uneeded changes
tomer-w Sep 9, 2024
5015038
Make the same perf optimization to the MQTT messages
tomer-w Sep 9, 2024
c0128d9
Fix MQTT tests
tomer-w Sep 9, 2024
5b6f176
prettier fix
tomer-w Sep 9, 2024
de5b9d6
Allow easy mocking of stub logger callTransports function to receive …
tomer-w Sep 11, 2024
ed7c82e
cleanup leftover
tomer-w Sep 11, 2024
75e81f8
Fix prettier issues
tomer-w Sep 11, 2024
df6e9f3
Merge branch 'dev' into master
tomer-w Sep 11, 2024
bb2c4be
Finalize logger mock structure and add few lambda log calls
tomer-w Sep 12, 2024
2081f9e
Merge branch 'master' of https://github.com/tomer-w/zigbee2mqtt
tomer-w Sep 12, 2024
b409d53
fix prettier issue
tomer-w Sep 12, 2024
1c88f02
Run prettier
tomer-w Sep 12, 2024
da7f8fb
Merge branch 'master' of https://github.com/tomer-w/zigbee2mqtt
tomer-w Sep 12, 2024
f7b88ff
Revert "fix prettier issue"
tomer-w Sep 12, 2024
f14d9f1
Update zigbee.ts
tomer-w Sep 12, 2024
113b299
Fix test issues
tomer-w Sep 12, 2024
48a2157
Revert await removal
tomer-w Sep 12, 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
4 changes: 2 additions & 2 deletions lib/mqtt.ts
Original file line number Diff line number Diff line change
Expand Up @@ -145,7 +145,7 @@ export default class MQTT {
@bind public onMessage(topic: string, message: Buffer): void {
// Since we subscribe to zigbee2mqtt/# we also receive the message we send ourselves, skip these.
if (!this.publishedTopics.has(topic)) {
logger.debug(`Received MQTT message on '${topic}' with data '${message.toString()}'`, NS);
logger.debug(() => `Received MQTT message on '${topic}' with data '${message.toString()}'`, NS);
this.eventBus.emitMQTTMessage({topic, message: message.toString()});
}

Expand Down Expand Up @@ -196,7 +196,7 @@ export default class MQTT {
}

if (!skipLog) {
logger.info(`MQTT publish: topic '${topic}', payload '${payload}'`, NS);
logger.info(() => `MQTT publish: topic '${topic}', payload '${payload}'`, NS);
Nerivec marked this conversation as resolved.
Show resolved Hide resolved
}

const actualOptions: mqtt.IClientPublishOptions = {...defaultOptions, ...options};
Expand Down
19 changes: 10 additions & 9 deletions lib/util/logger.ts
Original file line number Diff line number Diff line change
Expand Up @@ -189,32 +189,33 @@ class Logger {
return this.cachedNamespacedLevels[namespace];
}

private log(level: settings.LogLevel, message: string, namespace: string): void {
private log(level: settings.LogLevel, messageOrLambda: string | (() => string), namespace: string): void {
const nsLevel = this.cacheNamespacedLevel(namespace);

if (settings.LOG_LEVELS.indexOf(level) <= settings.LOG_LEVELS.indexOf(nsLevel)) {
const message: string = messageOrLambda instanceof Function ? messageOrLambda() : messageOrLambda;
this.logger.log(level, `${namespace}: ${message}`);
}
}

public error(message: string, namespace: string = 'z2m'): void {
this.log('error', message, namespace);
public error(messageOrLambda: string | (() => string), namespace: string = 'z2m'): void {
this.log('error', messageOrLambda, namespace);
}

public warning(message: string, namespace: string = 'z2m'): void {
this.log('warning', message, namespace);
public warning(messageOrLambda: string | (() => string), namespace: string = 'z2m'): void {
this.log('warning', messageOrLambda, namespace);
}

public info(message: string, namespace: string = 'z2m'): void {
this.log('info', message, namespace);
public info(messageOrLambda: string | (() => string), namespace: string = 'z2m'): void {
this.log('info', messageOrLambda, namespace);
}

public debug(message: string, namespace: string = 'z2m'): void {
public debug(messageOrLambda: string | (() => string), namespace: string = 'z2m'): void {
if (this.debugNamespaceIgnoreRegex?.test(namespace)) {
return;
}

this.log('debug', message, namespace);
this.log('debug', messageOrLambda, namespace);
}

// Cleanup any old log directory.
Expand Down
31 changes: 29 additions & 2 deletions test/controller.test.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
process.env.NOTIFY_SOCKET = 'mocked';
const data = require('./stub/data');
const logger = require('./stub/logger');
const Transport = require('winston-transport');
const zigbeeHerdsman = require('./stub/zigbeeHerdsman');
const MQTT = require('./stub/mqtt');
const path = require('path');
Expand Down Expand Up @@ -325,21 +326,47 @@ describe('Controller', () => {
});

it('Handle mqtt message', async () => {
class DummyTransport extends Transport {
log(info, callback) {}
}
const transport = new DummyTransport();
logger.setTransportsEnabled(true);
let spyTransport = jest.spyOn(transport, 'log').mockImplementation();
logger.addTransport(transport);

await controller.start();
logger.debug.mockClear();
await MQTT.events.message('dummytopic', 'dummymessage');
expect(logger.debug).toHaveBeenCalledWith("Received MQTT message on 'dummytopic' with data 'dummymessage'", LOG_MQTT_NS);
expect(spyTransport).toHaveBeenCalledWith(
{level: 'debug', message: "Received MQTT message on 'dummytopic' with data 'dummymessage'", namespace: LOG_MQTT_NS},
expect.any(Function),
);
logger.removeTransport(transport);
logger.setTransportsEnabled(false);
});

it('Skip MQTT messages on topic we published to', async () => {
class DummyTransport extends Transport {
log(info, callback) {}
}
const transport = new DummyTransport();
logger.setTransportsEnabled(true);
let spyTransport = jest.spyOn(transport, 'log').mockImplementation();
logger.addTransport(transport);

await controller.start();
logger.debug.mockClear();
await MQTT.events.message('zigbee2mqtt/skip-this-topic', 'skipped');
expect(logger.debug).toHaveBeenCalledWith("Received MQTT message on 'zigbee2mqtt/skip-this-topic' with data 'skipped'", LOG_MQTT_NS);
Copy link
Owner

Choose a reason for hiding this comment

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

Instead of having to do this can't we modify stub/logger.js to evaluate the lambda?

Copy link
Collaborator

@Nerivec Nerivec Sep 9, 2024

Choose a reason for hiding this comment

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

See https://github.com/Koenkk/zigbee2mqtt/pull/23893/files#r1750650326 I was thinking, ignoring the logging completely and spying on eventBus instead? Cleaner to test the code than the logs.

Copy link
Owner

Choose a reason for hiding this comment

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

For this specific test case it's possible, but there are also some test cases on which this cannot be used. So having the stub/logger.js modified would be nice anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did both:

  1. Changed the code to spy on eventBus as it really seems more robust.
  2. Changed the stub logger implementation so it will be easy to spy on the actual logger message. I added an example of this in the last line of the 'Handle mqtt message' test case so you can see that it is simple to do.

expect(spyTransport).toHaveBeenCalledWith(
{level: 'debug', message: "Received MQTT message on 'zigbee2mqtt/skip-this-topic' with data 'skipped'", namespace: LOG_MQTT_NS},
expect.any(Function),
);
logger.debug.mockClear();
await controller.mqtt.publish('skip-this-topic', '', {});
await MQTT.events.message('zigbee2mqtt/skip-this-topic', 'skipped');
expect(logger.debug).toHaveBeenCalledTimes(0);
logger.removeTransport(transport);
logger.setTransportsEnabled(false);
});

it('On zigbee event message', async () => {
Expand Down
3 changes: 3 additions & 0 deletions test/logger.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -164,6 +164,9 @@ describe('Logger', () => {
logger[level]('msg', 'abcd');
expect(logSpy).toHaveBeenLastCalledWith(level, 'abcd: msg');
expect(consoleWriteSpy).toHaveBeenCalledTimes(i++);
logger[level](() => 'func msg', 'abcd');
expect(logSpy).toHaveBeenLastCalledWith(level, 'abcd: func msg');
expect(consoleWriteSpy).toHaveBeenCalledTimes(i++);

for (const higherLevel of otherLevels.higher) {
logger[higherLevel]('higher msg');
Expand Down
11 changes: 6 additions & 5 deletions test/stub/logger.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,9 @@ let namespacedLevels = {};
let transports = [];

let transportsEnabled = false;
const callTransports = (level, message, namespace) => {
const callTransports = (level, messageOrLambda, namespace) => {
if (transportsEnabled) {
const message = messageOrLambda instanceof Function ? messageOrLambda() : messageOrLambda;
for (const transport of transports) {
transport.log({level, message, namespace}, () => {});
}
Expand All @@ -15,10 +16,10 @@ const callTransports = (level, message, namespace) => {

const mock = {
init: jest.fn(),
info: jest.fn().mockImplementation((msg, namespace = 'z2m') => callTransports('info', msg, namespace)),
warning: jest.fn().mockImplementation((msg, namespace = 'z2m') => callTransports('warning', msg, namespace)),
error: jest.fn().mockImplementation((msg, namespace = 'z2m') => callTransports('error', msg, namespace)),
debug: jest.fn().mockImplementation((msg, namespace = 'z2m') => callTransports('debug', msg, namespace)),
info: jest.fn().mockImplementation((messageOrLambda, namespace = 'z2m') => callTransports('info', messageOrLambda, namespace)),
warning: jest.fn().mockImplementation((messageOrLambda, namespace = 'z2m') => callTransports('warning', messageOrLambda, namespace)),
error: jest.fn().mockImplementation((messageOrLambda, namespace = 'z2m') => callTransports('error', messageOrLambda, namespace)),
debug: jest.fn().mockImplementation((messageOrLambda, namespace = 'z2m') => callTransports('debug', messageOrLambda, namespace)),
cleanup: jest.fn(),
logOutput: jest.fn(),
add: (transport) => transports.push(transport),
Expand Down