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 78.48837% with 37 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/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	70.00%	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 ⚠️
packages/cli/src/services/frontend.service.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

packages/core/src/utils.ts

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.

packages/core/src/utils.ts

Follow-up to: #12310 (comment)

ivov

Changes look good. There are some failing tests, likely needs merging master.

ivov

Workflow tests seem to be failing for other PRs so I expect they're unrelated: https://github.com/n8n-io/n8n/actions/runs/12432707679/job/34712821182

github-actions · 2024-12-23T11:18:20Z

⚠️ Some Cypress E2E specs are failing, please fix them before merging

cypress · 2024-12-23T11:18:30Z

n8n Run #8464

Run Properties: Failed #8464 • 471d7b9420: 🌳 master 🖥️ browsers:node18.12.0-chrome107 🤖 PR User 🗃️ e2e/*

Project	`n8n`
Branch Review	`master`
Run status	`Failed #8464`
Run duration	`04m 14s`
Commit	`471d7b9420: 🌳 master 🖥️ browsers:node18.12.0-chrome107 🤖 PR User 🗃️ e2e/*`
Committer	`कारतोफ्फेलस्क्रिप्ट™`
View all properties for this run ↗︎

Test results
Failures	`1`
Flaky	`2`
Pending	`0`
Skipped	`0`
Passing	`276`
View all changes introduced in this branch ↗︎

Tests for review

14-mapping.cy.ts • 1 failed test

View Output Video

Test		Artifacts
Data mapping > maps expressions from json view		`Test Replay` `Screenshots` `Video`

45-workflow-selector-parameter.cy.ts • 0 failed tests

View Output

Test		Artifacts

20-workflow-executions.cy.ts • 0 failed tests

View Output

Test		Artifacts

30-editor-after-route-changes.cy.ts • 0 failed tests

View Output

Test		Artifacts

48-subworkflow-inputs.cy.ts • 0 failed tests

View Output

Test		Artifacts

The first 5 failed specs are shown, see all 52 specs in Cypress Cloud.

45-ai-assistant.cy.ts • 1 flaky test

View Output Video

Test		Artifacts
AI Assistant::enabled > should resize assistant chat down		`Test Replay` `Screenshots` `Video`

19-execution.cy.ts • 1 flaky test

View Output Video

Test		Artifacts
Execution > connections should be colored differently for pinned data > when executing a node		`Test Replay` `Screenshots` `Video`

github-actions · 2024-12-23T11:44:01Z

✅ All Cypress E2E specs passed

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

ivov reviewed Dec 23, 2024

View reviewed changes

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

c21c119

netroy requested a review from ivov December 23, 2024 09:49

fix formatting

9e0deca

ivov approved these changes Dec 23, 2024

View reviewed changes

netroy merged commit 471d7b9 into master Dec 23, 2024
43 of 44 checks passed

netroy deleted the move-logger-to-core branch December 23, 2024 12:46

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 20, 2024

netroy Dec 20, 2024

ivov Dec 20, 2024 •

edited

Loading

netroy Dec 20, 2024

ivov Dec 20, 2024

ivov left a comment

ivov left a comment

github-actions bot commented Dec 23, 2024

cypress bot commented Dec 23, 2024 •

edited

Loading

github-actions bot commented Dec 23, 2024

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

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

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

ivov left a comment

Choose a reason for hiding this comment

ivov left a comment

Choose a reason for hiding this comment

github-actions bot commented Dec 23, 2024

cypress bot commented Dec 23, 2024 • edited Loading

Tests for review

github-actions bot commented Dec 23, 2024

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

cypress bot commented Dec 23, 2024 •

edited

Loading