refactor(core): Move Logger to `core` (no-changelog) #12310

netroy · 2024-12-19T12:44:50Z

Summary

This PR moved the Logger to n8n-core, so that we can stop using console.* and Loggerproxy in more places.

Review / Merge checklist

PR title and summary are descriptive. (conventions)
Docs updated or follow-up ticket created.
Tests included.
PR Labeled with release/backport (if the PR is an urgent fix that needs to be backported)

codecov · 2024-12-19T13:16:49Z

Codecov Report

Attention: Patch coverage is 76.43678% with 41 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
packages/cli/src/config/index.ts	36.36%	7 Missing ⚠️
packages/core/src/NodeExecuteFunctions.ts	22.22%	7 Missing ⚠️
packages/core/src/ActiveWorkflows.ts	0.00%	5 Missing ⚠️
packages/core/src/InstanceSettings.ts	66.66%	3 Missing and 1 partial ⚠️
packages/nodes-base/nodes/Ldap/Ldap.node.ts	0.00%	4 Missing ⚠️
packages/core/src/logging/logger.ts	66.66%	3 Missing ⚠️
packages/cli/src/help.ts	0.00%	2 Missing ⚠️
packages/core/src/InstanceSettingsConfig.ts	60.00%	0 Missing and 2 partials ⚠️
packages/@n8n/task-runner/src/start.ts	0.00%	1 Missing ⚠️
.../src/controllers/workflow-statistics.controller.ts	0.00%	1 Missing ⚠️
... and 5 more

📢 Thoughts on this report? Let us know!

ivov

Thanks for taking this on! Code looks good, will give it a spin soon!

PS: I think we can omit (no-changelog) for refactor PRs

packages/cli/src/config/index.ts

packages/core/src/InstanceSettingsConfig.ts

ivov · 2024-12-19T15:19:20Z

packages/core/src/logging/logger.ts

 			if (!this.isScopingEnabled) return info;

-			const { scopes } = info.metadata;
+			const { scopes } = (info as unknown as { metadata: LogMetadata }).metadata;


Why is this better than the original?

not sure why, but I'm getting this error

So confused, this should've been erroring out in cli then? Also the TransformableInfo type is left open [key: string | symbol]: any; so worst case we shouldn't need the unknown intermediary or? info.metadata as LogMetadata

ivov · 2024-12-19T15:20:14Z

packages/core/src/utils.ts

@@ -0,0 +1,3 @@
+export function isObjectLiteral(item: unknown): item is { [key: string]: unknown } {


Nit - better naming

Suggested change

export function isObjectLiteral(item: unknown): item is { [key: string]: unknown } {

export function isObjectLiteral(candidate: unknown): candidate is { [key: string]: unknown } {

we have an exact copy of this in cli. if we rename this, we should rename it in the other copy as well, so that if/when we deduplicate this code, it's not confusing for the person doing the refactoring.

Since we're here let's keep only one copy in core and import it from elsewhere?

ivov · 2024-12-19T15:22:54Z

packages/workflow/src/Interfaces.ts

+export type LogMetadata = {
+	[key: string]: unknown;
+	scopes?: LogScope[];
+	file?: string;
+	function?: string;
+};
+export type Logger = Record<
+	Exclude<LogLevel, 'silent'>,
+	(message: string, metadata?: LogMetadata) => void
+>;
+export type LogLocationMetadata = Pick<LogMetadata, 'file' | 'function'>;


Instead of picking from LogMetadata I wonder if it'd be more readable to define LogLocationMetadata as those two properties and intersect this type to create LogMetadata?

I'm not clear just from this what Logger is here as opposed to the Logger service. The proxy? Maybe we can docline this?

I just copied this code over. I think we can refactor this bit in the next step when we get rid of more of the LoggerProxy code.

ivov · 2024-12-20T08:39:40Z

packages/core/src/logging/logger.ts

 			if (!this.isScopingEnabled) return info;

-			const { scopes } = info.metadata;
+			const { scopes } = (info as unknown as { metadata: LogMetadata }).metadata;


So confused, this should've been erroring out in cli then? Also the TransformableInfo type is left open [key: string | symbol]: any; so worst case we shouldn't need the unknown intermediary or? info.metadata as LogMetadata

ivov · 2024-12-20T08:40:08Z

packages/core/src/logging/logger.ts

Why remove the .service. infix?

I prefer the file name and the DI class names to align. I can add the .service back, but then I prefer to rename the class to LoggerService. Would that be acceptable?

Turning all the logger references to loggerService makes the code everywhere more verbose for little gain. Do we need to be so consistent? I always thought of the -Service suffix as an easy way to name classes that otherwise would be hard to name like WorklowService but that doesn't really apply for the logger.

If the alternative is making all code more verbose, I'd rather omit the .service. infix here. But I also notice that this is also inconsistent in that we're having some DI services infixed and some not.

we don't need to rename all variables. I just meant the class. So the only change would be change the class name in this file to LoggerService, and renaming the file back to logger.service.ts.
however I personally the Service suffix in this particular case not very useful.

however I personally the Service suffix in this particular case not very useful.

Agree! In any case if this renaming won't cause lots of changes everywhere, I'd be happy with what you decide.

ivov · 2024-12-20T08:41:11Z

packages/core/src/utils.ts

@@ -0,0 +1,3 @@
+export function isObjectLiteral(item: unknown): item is { [key: string]: unknown } {


Since we're here let's keep only one copy in core and import it from elsewhere?

Follow-up to: #12310 (comment)

refactor(core): Move Logger to core (no-changelog)

551163b

n8n-assistant bot added core Enhancement outside /nodes-base and /editor-ui n8n team Authored by the n8n team labels Dec 19, 2024

netroy requested a review from ivov December 19, 2024 14:56

ivov reviewed Dec 19, 2024

View reviewed changes

netroy added 2 commits December 19, 2024 17:16

Merge remote-tracking branch 'origin/master' into move-logger-to-core

7530f13

address PR comments

18d67cc

netroy requested a review from ivov December 19, 2024 16:29

ivov reviewed Dec 20, 2024

View reviewed changes

ivov added a commit that referenced this pull request Dec 20, 2024

refactor: Introduce explicit-member-accessibility lint rule

d0ce3af

Follow-up to: #12310 (comment)

ivov mentioned this pull request Dec 20, 2024

refactor: Introduce explicit-member-accessibility lint rule #12325

Merged

netroy added 2 commits December 20, 2024 14:47

Merge remote-tracking branch 'origin/master' into move-logger-to-core

08d7014

deduplicate isObjectLiteral, add docs, and tests

74e3a59

netroy mentioned this pull request Dec 20, 2024

refactor(core): Deduplicate isObjectLiteral, add docs and tests #12332

Merged

4 tasks

Merge remote-tracking branch 'origin/master' into move-logger-to-core

3acf313

netroy requested a review from a team as a code owner December 20, 2024 15:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(core): Move Logger to `core` (no-changelog) #12310

refactor(core): Move Logger to `core` (no-changelog) #12310

netroy commented Dec 19, 2024

codecov bot commented Dec 19, 2024 •

edited

Loading

ivov left a comment

ivov Dec 19, 2024

netroy Dec 19, 2024

ivov Dec 20, 2024

ivov Dec 19, 2024

netroy Dec 19, 2024

ivov Dec 20, 2024

ivov Dec 19, 2024

netroy Dec 19, 2024

ivov Dec 20, 2024

ivov Dec 20, 2024

netroy Dec 20, 2024

ivov Dec 20, 2024 •

edited

Loading

netroy Dec 20, 2024

ivov Dec 20, 2024

ivov Dec 20, 2024

		@@ -0,0 +1,3 @@
		export function isObjectLiteral(item: unknown): item is { [key: string]: unknown } {

	export function isObjectLiteral(item: unknown): item is { [key: string]: unknown } {
	export function isObjectLiteral(candidate: unknown): candidate is { [key: string]: unknown } {

refactor(core): Move Logger to core (no-changelog) #12310

Are you sure you want to change the base?

refactor(core): Move Logger to core (no-changelog) #12310

Conversation

netroy commented Dec 19, 2024

Summary

Review / Merge checklist

codecov bot commented Dec 19, 2024 • edited Loading

Codecov Report

ivov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ivov Dec 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

refactor(core): Move Logger to `core` (no-changelog) #12310

refactor(core): Move Logger to `core` (no-changelog) #12310

codecov bot commented Dec 19, 2024 •

edited

Loading

ivov Dec 20, 2024 •

edited

Loading