-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Consolidate logging and introduce auto-prefix #7731
Changes from all commits
3651a82
b8a81c6
4c7177b
c7216a6
b126592
1dec61f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,28 +2,116 @@ | |
* Copyright (c) OpenLens Authors. All rights reserved. | ||
* Licensed under MIT License. See LICENSE in root directory for more information. | ||
*/ | ||
import { getInjectable, getInjectionToken } from "@ogre-tools/injectable"; | ||
import { kebabCase, toUpper } from "lodash/fp"; | ||
import { | ||
DiContainerForInjection, | ||
getInjectable, | ||
getInjectionToken, | ||
lifecycleEnum, | ||
} from "@ogre-tools/injectable"; | ||
import type { Logger } from "./logger"; | ||
import { winstonLoggerInjectable } from "./winston-logger.injectable"; | ||
import { pipeline } from "@ogre-tools/fp"; | ||
|
||
/** @deprecated Use specific injectionToken, eg. logErrorInjectionToken */ | ||
export const loggerInjectionToken = getInjectionToken<Logger>({ | ||
id: "logger-injection-token", | ||
}); | ||
|
||
export const loggerInjectable = getInjectable({ | ||
id: "logger", | ||
instantiate: (di): Logger => { | ||
const baseLogger = di.inject(winstonLoggerInjectable); | ||
|
||
return { | ||
debug: (message, ...data) => baseLogger.debug(message, ...data), | ||
info: (message, ...data) => baseLogger.info(message, ...data), | ||
warn: (message, ...data) => baseLogger.warn(message, ...data), | ||
error: (message, ...data) => baseLogger.error(message, ...data), | ||
silly: (message, ...data) => baseLogger.silly(message, ...data), | ||
}; | ||
}, | ||
instantiate: (di): Logger => ({ | ||
debug: di.inject(logDebugInjectionToken), | ||
info: di.inject(logInfoInjectionToken), | ||
warn: di.inject(logWarningInjectionToken), | ||
error: di.inject(logErrorInjectionToken), | ||
silly: di.inject(logSillyInjectionToken), | ||
}), | ||
|
||
decorable: false, | ||
injectionToken: loggerInjectionToken, | ||
}); | ||
|
||
export type LogFunction = (message: string, ...data: any[]) => void; | ||
|
||
export const logDebugInjectionToken = getInjectionToken<LogFunction>({ | ||
id: "log-debug-injection-token", | ||
}); | ||
|
||
export const logInfoInjectionToken = getInjectionToken<LogFunction>({ | ||
id: "log-info-injection-token", | ||
}); | ||
|
||
export const logWarningInjectionToken = getInjectionToken<LogFunction>({ | ||
id: "log-warning-injection-token", | ||
}); | ||
|
||
export const logErrorInjectionToken = getInjectionToken<LogFunction>({ | ||
id: "log-error-injection-token", | ||
}); | ||
|
||
export const logSillyInjectionToken = getInjectionToken<LogFunction>({ | ||
id: "log-silly-injection-token", | ||
}); | ||
|
||
const screamingKebabCase = (str: string) => pipeline(str, kebabCase, toUpper); | ||
|
||
const getLogFunctionFor = | ||
(scenario: keyof Logger) => | ||
(di: DiContainerForInjection): LogFunction => { | ||
const winstonLogger = di.inject(winstonLoggerInjectable); | ||
|
||
return (message, ...data) => { | ||
winstonLogger[scenario]( | ||
di.sourceNamespace | ||
? `[${screamingKebabCase(di.sourceNamespace)}]: ${message}` | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Notice: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why not use the already existing There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Because after this you don't have to worry about prefixing at all anymore. It's all done automatically within There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Because of inconsistency, boilerplate and unnecessary complexity. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. More interestingly, I'm thinking how
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. profile the loading time of the app so we can get closer to sub 2second loading |
||
: message, | ||
...data | ||
); | ||
}; | ||
}; | ||
|
||
export const logDebugInjectable = getInjectable({ | ||
id: "log-debug", | ||
instantiate: getLogFunctionFor("debug"), | ||
injectionToken: logDebugInjectionToken, | ||
lifecycle: lifecycleEnum.keyedSingleton({ | ||
getInstanceKey: (di) => di.sourceNamespace, | ||
}), | ||
}); | ||
|
||
export const logInfoInjectable = getInjectable({ | ||
id: "log-info", | ||
instantiate: getLogFunctionFor("info"), | ||
injectionToken: logInfoInjectionToken, | ||
lifecycle: lifecycleEnum.keyedSingleton({ | ||
getInstanceKey: (di) => di.sourceNamespace, | ||
}), | ||
}); | ||
|
||
export const logWarningInjectable = getInjectable({ | ||
id: "log-warning", | ||
instantiate: getLogFunctionFor("warn"), | ||
injectionToken: logWarningInjectionToken, | ||
lifecycle: lifecycleEnum.keyedSingleton({ | ||
getInstanceKey: (di) => di.sourceNamespace, | ||
}), | ||
}); | ||
|
||
export const logErrorInjectable = getInjectable({ | ||
id: "log-error", | ||
instantiate: getLogFunctionFor("error"), | ||
injectionToken: logErrorInjectionToken, | ||
lifecycle: lifecycleEnum.keyedSingleton({ | ||
getInstanceKey: (di) => di.sourceNamespace, | ||
}), | ||
}); | ||
|
||
export const logSillyInjectable = getInjectable({ | ||
id: "log-silly", | ||
instantiate: getLogFunctionFor("silly"), | ||
injectionToken: logSillyInjectionToken, | ||
lifecycle: lifecycleEnum.keyedSingleton({ | ||
getInstanceKey: (di) => di.sourceNamespace, | ||
}), | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,78 @@ | ||
import { createContainer, getInjectable } from "@ogre-tools/injectable"; | ||
import { registerFeature } from "@k8slens/feature-core"; | ||
import { loggerFeature } from "./feature"; | ||
import { winstonLoggerInjectable } from "./winston-logger.injectable"; | ||
|
||
import { | ||
logDebugInjectionToken, | ||
logErrorInjectionToken, | ||
logInfoInjectionToken, | ||
logSillyInjectionToken, | ||
logWarningInjectionToken, | ||
} from "./logger.injectable"; | ||
|
||
import { getFeature } from "@k8slens/feature-core/src/feature"; | ||
|
||
describe("logger", () => { | ||
[ | ||
{ scenario: "debug", injectionToken: logDebugInjectionToken }, | ||
{ scenario: "info", injectionToken: logInfoInjectionToken }, | ||
{ scenario: "warn", injectionToken: logWarningInjectionToken }, | ||
{ scenario: "error", injectionToken: logErrorInjectionToken }, | ||
{ scenario: "silly", injectionToken: logSillyInjectionToken }, | ||
].forEach(({ scenario, injectionToken }) => { | ||
it(`given not inside a Feature, when logging "${scenario}", does so without a prefix`, () => { | ||
const di = createContainer("irrelevant"); | ||
|
||
registerFeature(di, loggerFeature); | ||
|
||
const winstonLoggerStub = { [scenario]: jest.fn() } as any; | ||
|
||
di.override(winstonLoggerInjectable, () => winstonLoggerStub); | ||
|
||
const logScenario = di.inject(injectionToken); | ||
|
||
logScenario("some-message", "some-data"); | ||
|
||
expect(winstonLoggerStub[scenario]).toHaveBeenCalledWith( | ||
"some-message", | ||
"some-data" | ||
); | ||
}); | ||
|
||
it(`given inside a Feature, when logging "${scenario}", does so with Feature's id as prefix`, () => { | ||
const di = createContainer("irrelevant"); | ||
|
||
const logScenarioInFeature = getInjectable({ | ||
id: "some-functionality", | ||
instantiate: (di) => di.inject(injectionToken), | ||
}); | ||
|
||
|
||
const someFeature = getFeature({ | ||
id: "some-feature", | ||
|
||
register: (di) => { | ||
di.register(logScenarioInFeature); | ||
}, | ||
|
||
dependencies: [loggerFeature], | ||
}); | ||
|
||
registerFeature(di, someFeature); | ||
|
||
const winstonLoggerStub = { [scenario]: jest.fn() } as any; | ||
|
||
di.override(winstonLoggerInjectable, () => winstonLoggerStub); | ||
|
||
const logScenario = di.inject(logScenarioInFeature); | ||
|
||
logScenario("some-message", "some-data"); | ||
|
||
expect(winstonLoggerStub[scenario]).toHaveBeenCalledWith( | ||
"[SOME-FEATURE]: some-message", | ||
"some-data" | ||
); | ||
}); | ||
}); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also why are these categorically better?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Depending from something that you don't use completely is considered as bad practice. (Interface segregation principle) E.g. if you depend from
logger
which is object containing multiple methods, you'll likely use only one of them. This means that e.g. in the unit tests you have to override stuff that you are not interested.