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

Consolidate logging and introduce auto-prefix #7731

Merged
merged 6 commits into from
May 17, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
10 changes: 5 additions & 5 deletions open-lens/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -199,11 +199,11 @@
"@k8slens/run-many": "^1.0.0-alpha.7",
"@k8slens/startable-stoppable": "^1.0.0-alpha.6",
"@k8slens/utilities": "^1.0.0-alpha.6",
"@ogre-tools/fp": "^15.8.1",
"@ogre-tools/injectable": "^15.8.1",
"@ogre-tools/injectable-extension-for-auto-registration": "^15.8.1",
"@ogre-tools/injectable-extension-for-mobx": "^15.8.1",
"@ogre-tools/injectable-react": "^15.8.1",
"@ogre-tools/fp": "^16.1.0",
"@ogre-tools/injectable": "^16.1.0",
"@ogre-tools/injectable-extension-for-auto-registration": "^16.1.0",
"@ogre-tools/injectable-extension-for-mobx": "^16.1.0",
"@ogre-tools/injectable-react": "^16.1.0",
"mobx": "^6.9.0"
},
"devDependencies": {
Expand Down
215 changes: 114 additions & 101 deletions package-lock.json

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@
"@types/react": "^17"
},
"devDependencies": {
"@ogre-tools/linkable": "^15.8.1",
"@ogre-tools/linkable": "^16.1.0",
"adr": "^1.4.3",
"cross-env": "^7.0.3",
"lerna": "^6.6.1",
Expand Down
8 changes: 4 additions & 4 deletions packages/business-features/keyboard-shortcuts/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -33,10 +33,10 @@
"peerDependencies": {
"@k8slens/feature-core": "^6.5.0-alpha.0",
"@k8slens/react-application": "^1.0.0-alpha.0",
"@ogre-tools/fp": "^15.8.1",
"@ogre-tools/injectable": "^15.8.1",
"@ogre-tools/injectable-extension-for-auto-registration": "^15.8.1",
"@ogre-tools/injectable-react": "^15.8.1",
"@ogre-tools/fp": "^16.1.0",
"@ogre-tools/injectable": "^16.1.0",
"@ogre-tools/injectable-extension-for-auto-registration": "^16.1.0",
"@ogre-tools/injectable-react": "^16.1.0",
"lodash": "^4.17.21",
"react": "^17 || ^18"
},
Expand Down
2 changes: 1 addition & 1 deletion packages/cluster-settings/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,6 @@
"rimraf": "^4.4.1"
},
"peerDependencies": {
"@ogre-tools/injectable": "^15.8.1"
"@ogre-tools/injectable": "^16.1.0"
}
}
12 changes: 6 additions & 6 deletions packages/core/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -188,7 +188,7 @@
"@async-fn/jest": "1.6.4",
"@k8slens/messaging-fake-bridge": "^1.0.0-alpha.7",
"@k8slens/react-testing-library-discovery": "^1.0.0-alpha.4",
"@ogre-tools/linkable": "^15.8.1",
"@ogre-tools/linkable": "^16.1.0",
"@sentry/types": "^6.19.7",
"@swc/cli": "^0.1.62",
"@swc/core": "^1.3.53",
Expand Down Expand Up @@ -311,11 +311,11 @@
"@k8slens/test-utils": "^1.0.0-alpha.3",
"@k8slens/tooltip": "^1.0.0-alpha.5",
"@k8slens/utilities": "^1.0.0-alpha.1",
"@ogre-tools/fp": "^15.8.1",
"@ogre-tools/injectable": "^15.8.1",
"@ogre-tools/injectable-extension-for-auto-registration": "^15.8.1",
"@ogre-tools/injectable-extension-for-mobx": "^15.8.1",
"@ogre-tools/injectable-react": "^15.8.1",
"@ogre-tools/fp": "^16.1.0",
"@ogre-tools/injectable": "^16.1.0",
"@ogre-tools/injectable-extension-for-auto-registration": "^16.1.0",
"@ogre-tools/injectable-extension-for-mobx": "^16.1.0",
"@ogre-tools/injectable-react": "^16.1.0",
"mobx": "^6.9.0",
"mobx-observable-history": "^2.0.3",
"mobx-react": "^7.6.0",
Expand Down
10 changes: 5 additions & 5 deletions packages/infrastructure/webpack/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@
"test:unit": "jest --coverage --runInBand"
},
"dependencies": {
"@ogre-tools/linkable": "^15.8.1",
"@ogre-tools/linkable": "^16.1.0",
"@types/webpack-env": "^1.18.0",
"css-loader": "^6.7.2",
"fork-ts-checker-webpack-plugin": "^7.3.0",
Expand All @@ -40,15 +40,15 @@
"webpack-cli": "^4.10.0"
},
"peerDependencies": {
"@ogre-tools/fp": "^15.8.1",
"@ogre-tools/injectable": "^15.8.1",
"@ogre-tools/fp": "^16.1.0",
"@ogre-tools/injectable": "^16.1.0",
"lodash": "^4.17.21"
},
"devDependencies": {
"@async-fn/jest": "^1.6.4",
"@k8slens/typescript": "^6.5.0-alpha.3",
"@ogre-tools/injectable-extension-for-auto-registration": "^15.9.0",
"@ogre-tools/test-utils": "^15.8.1",
"@ogre-tools/injectable-extension-for-auto-registration": "^16.1.0",
"@ogre-tools/test-utils": "^16.1.0",
"ts-node": "^10.9.1",
"webpack-node-externals": "^3.0.0"
},
Expand Down
2 changes: 1 addition & 1 deletion packages/kube-object/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@
},
"peerDependencies": {
"@k8slens/utilities": "^1.0.0-alpha.2",
"@ogre-tools/injectable": "^15.8.1",
"@ogre-tools/injectable": "^16.1.0",
"auto-bind": "^4.0.0",
"moment": "^2.29.4",
"rfc6902": "^5.0.1",
Expand Down
2 changes: 1 addition & 1 deletion packages/kubectl-versions/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@
"test": "jest --coverage --runInBand"
},
"peerDependencies": {
"@ogre-tools/injectable": "^15.8.1"
"@ogre-tools/injectable": "^16.1.0"
},
"devDependencies": {
"@k8slens/webpack": "^6.5.0-alpha.8",
Expand Down
2 changes: 1 addition & 1 deletion packages/legacy-global-di/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@
"lint:fix": "lens-lint --fix"
},
"peerDependencies": {
"@ogre-tools/injectable": "^15.8.1"
"@ogre-tools/injectable": "^16.1.0"
},
"devDependencies": {
"@k8slens/eslint-config": "^6.5.0-alpha.3",
Expand Down
2 changes: 1 addition & 1 deletion packages/list-layout/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@
"peerDependencies": {
"@k8slens/kube-object": "^1.0.0-alpha.1",
"@k8slens/utilities": "^1.0.0-alpha.3",
"@ogre-tools/injectable": "^15.8.1",
"@ogre-tools/injectable": "^16.1.0",
"react": "^17.0.2"
},
"devDependencies": {
Expand Down
11 changes: 11 additions & 0 deletions packages/logger/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,19 @@
* Licensed under MIT License. See LICENSE in root directory for more information.
*/

export type { LogFunction } from "./src/logger.injectable";
export {
logDebugInjectionToken,
logErrorInjectionToken,
logInfoInjectionToken,
logSillyInjectionToken,
logWarningInjectionToken,
} from "./src/logger.injectable";

export type { Logger } from "./src/logger";
/** @deprecated Use specific injectionToken, eg. logErrorInjectionToken */
export { loggerInjectionToken } from "./src/logger.injectable";
/** @deprecated Use specific injectionToken, eg. logErrorInjectionToken */
export { prefixedLoggerInjectable } from "./src/prefixed-logger.injectable";
export { loggerTransportInjectionToken } from "./src/transports";
export { winstonLoggerInjectable } from "./src/winston-logger.injectable";
Expand Down
13 changes: 9 additions & 4 deletions packages/logger/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -24,18 +24,23 @@
"license": "MIT",
"homepage": "https://github.com/lensapp/lens",
"scripts": {
"build": "webpack",
"build": "lens-webpack-build",
"test:unit": "jest --coverage --runInBand",
"lint": "lens-lint",
"lint:fix": "lens-lint --fix"
},
"peerDependencies": {
"@k8slens/feature-core": "^6.5.0-alpha.0",
"@ogre-tools/injectable": "^15.8.1",
"@ogre-tools/injectable-extension-for-auto-registration": "^15.3.0",
"@ogre-tools/fp": "^16.1.0",
"@ogre-tools/injectable": "^16.1.0",
"@ogre-tools/injectable-extension-for-auto-registration": "^16.1.0",
"lodash": "^4.17.21",
"winston": "^3.8.2"
},
"devDependencies": {
"@k8slens/eslint-config": "6.5.0-alpha.1",
"@k8slens/react-testing-library-discovery": "^1.0.0-alpha.4"
"@k8slens/jest": "^6.5.0-alpha.5",
"@k8slens/react-testing-library-discovery": "^1.0.0-alpha.4",
"@k8slens/webpack": "^6.5.0-alpha.6"
}
}
112 changes: 100 additions & 12 deletions packages/logger/src/logger.injectable.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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",
});
Comment on lines +35 to +55
Copy link
Collaborator

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?

Copy link
Contributor

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.


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}`
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Notice: di.sourceNamespace is a new thing in injectable, making this auto-prefixing possible.

Copy link
Collaborator

Choose a reason for hiding this comment

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

why not use the already existing prefixedLogger?

Copy link
Contributor

Choose a reason for hiding this comment

The 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 loggerFeature. Usage doesn't have to know about it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because of inconsistency, boilerplate and unnecessary complexity.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

More interestingly, I'm thinking how di.sourceNamespace could be leveraged elsewhere. With it, we can have stuff that is guaranteed to have a Feature-specific instance.

  1. We can simplify injections eg. for less required instantiationParameters.
  2. We can have error handling that can identify problematic Features.
  3. We can have profiling that can identify slow Features.
  4. We can have Features able to only access its own directory in file-system.
  5. We can solve many naming collision issues, as namespace is always guaranteed unique.
  6. ...best ideas win a prize at next team event ;)

Copy link
Contributor

@gabriel-mirantis gabriel-mirantis May 17, 2023

Choose a reason for hiding this comment

The 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
and send that someplace that can be seen from metabase.
I'd like to be able to see the mean load time across the userbase by release

: 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,
}),
});
78 changes: 78 additions & 0 deletions packages/logger/src/logger.test.ts
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"
);
});
});
});
Loading