fix(core): Ensure LoggerProxy is not scoped
#11379
Conversation
n8n-assistant
bot
added
core
| @@ -61,7 +62,7 @@ export class Logger { | |||
| /** Create a logger that injects the given scopes into its log metadata. */ | |||
| scoped(scopes: LogScope | LogScope[]) { | |||
| scopes = Array.isArray(scopes) ? scopes : [scopes]; | |||
| const scopedLogger = new Logger(this.globalConfig, this.instanceSettings); | |||
| const scopedLogger = new Logger(this.globalConfig, this.instanceSettings, { isRoot: false }); | |||
There was a problem hiding this comment.
do you think it might make sense to move the minimal amount of code needed for scoped loggers to a separate class, instead of reusing this class for two separate purposes?
There was a problem hiding this comment.
Interesting, let me try this and report back
There was a problem hiding this comment.
Tried this quickly here. The scoped logger relies heavily on the existing logger, so
- if the scoped logger doesn't extend from the existing logger, we'd have to extract quite a bit of deeply integrated logic or duplicate quite a bit, also using a separate scoped logger like this introduces a circular dependency
ReferenceError: Circular dependency: TargetServer. Index: 0that I haven't spent time debugging yet - if the scoped logger does extend from the existing logger to reuse logic and avoid extracting most of it, then we're back at wanting a way to avoid initializing the proxy on every constructor call, which is what this PR is addressing
Let me know if worth investing more time into this
There was a problem hiding this comment.
does the scoped logger also need to use winston to write logs to the file?
also, if you prefer, we can merge this as it is, and I can have a look at splitting this in a separate PR.
There was a problem hiding this comment.
does the scoped logger also need to use winston to write logs to the file?
Yes, the scoped logger does everything the existing logger does, with the only addition that it auto-injects the scope string into the metadata of every log it emits, no matter the transport, etc.
also, if you prefer, we can merge this as it is, and I can have a look at splitting this in a separate PR.
Let's do this and revisit. I'd like to fix this minor bug quickly.
n8n
|
||||||||||||||||||||||||||||
| Project |
n8n
|
| Branch Review |
ensure-logger-proxy-is-not-scoped
|
| Run status |
|
| Run duration | 04m 12s |
| Commit |
|
| Committer | Iván Ovejero |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
Failures
|
0
|
Flaky
|
0
|
Pending
|
0
|
Skipped
|
0
|
Passing
|
458
|
| View all changes introduced in this branch ↗︎ | |
|
✅ All Cypress E2E specs passed |
|
Got released with |
Currently we initialize a
Loggersingleton shared by most services. More recently, we allowed some services to useLogger.scopedto instantiate a scoped logger to auto-inject scopes like'waiting-executions'into the scoped logger's log metadata.In
Logger.constructorwe also initialize aLoggerProxyfor use byworkflowandcorepackages. SinceLogger.scopedcallsLogger.constructorwe are unintentionally re-initializingLoggerProxyon everyLogger.scopedcall, which causesLoggerProxyto end up with the scopes of the lastLogger.scopedcall, e.g.LoggerProxyincorein regular mode is currently auto-injecting thewaiting-executionsscope.This PR ensures
LoggerProxyis only initialized by the rootLogger.constructorso thatLoggerProxydoes not receive scopes from further constructor calls.