feat(core): add customizable LoggerContext with labeling support
#4233
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## v6 #4233 +/- ##
==========================================
- Coverage 99.48% 99.47% -0.01%
==========================================
Files 219 219
Lines 14361 14367 +6
Branches 3277 3281 +4
==========================================
+ Hits 14287 14292 +5
- Misses 73 74 +1
Partials 1 1
... and 3 files with indirect coverage changes Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
SpencerKaiser
force-pushed
the
logger-context
branch
from
0bbb74c
to
ca40c9a
Compare
|
@B4nan I’m having a really difficult time tracking how data passes through the different abstract classes and where the context actually gets passed to The changes to Any idea what I’m doing wrong there? Any suggestions on the approach? |
There was a problem hiding this comment.
i am usually going the old school way, compile the package and just copy paste the dist folder inside node_modules, maybe that will work? the error sounds like you have some private property with different type, maybe you need to link more packages than just core? as you did changes in knex package too
SpencerKaiser
force-pushed
the
logger-context
branch
from
7cb9deb
to
c8dad69
Compare
SpencerKaiser
force-pushed
the
logger-context
branch
7 times, most recently
from
e10eba6
to
620c5d8
Compare
SpencerKaiser
force-pushed
the
logger-context
branch
from
620c5d8
to
35f33f6
Compare
packages/core/src/logging/Logger.ts
Outdated
| @@ -50,3 +51,5 @@ export interface LoggerOptions { | |||
| highlighter?: Highlighter; | |||
| usesReplicas?: boolean; | |||
| } | |||
|
|
|||
| export type LoggerContext = Pick<LogContext, 'label'>; | |||
There was a problem hiding this comment.
maybe better this way so its universal way to pass params down to the logger?
| export type LoggerContext = Pick<LogContext, 'label'>; | |
| export type LoggerContext = { label?: string } & Dictionary; |
and add a note about that to the section about custom loggers
There was a problem hiding this comment.
This change was also needed for LogContext... we might consider refactoring these at some point... one acts as a sort of public type where the other is more focused on internals unless someone uses LoggerFactory.
There was a problem hiding this comment.
Also I personally prefer picking the label just to keep as much overlap as possible. If you still want that to be unique on it's own, let me know and I can fix it
| @@ -608,7 +610,7 @@ export class QueryBuilder<T extends object = AnyEntity> { | |||
| } | |||
|
|
|||
| const type = this.connectionType || (method === 'run' ? 'write' : 'read'); | |||
| const res = await this.driver.getConnection(type).execute(query.sql, query.bindings as any[], method, this.context); | |||
There was a problem hiding this comment.
the same needs to be supported in mongo driver
There was a problem hiding this comment.
See my comment below 😬
B4nan
changed the title
LoggerContext to allow for custom labels in log outputLoggerContext with labeling support
|
Will you have time to do the mongo support? We can merge without it too, I'll be happy to do it myself. Planning to ship v5.7 tomorrow probably, and want to rebase v6 again afterward, so would be good to merge this before instead of battling the rebases again :] |
|
@B4nan sorry for the delay, I went on a mini vacation for the weekend + today; I'll work on it here shortly and/or tomorrow. I might have a question or two about it, but if I do I'll follow up in Slack for expediency! Re: rebasing; feel free to rebase main, just don't pull it into this branch... when that happens, I'll see the "Update branch" button and know to reset/stash/delete/recreate 😬 |
|
@B4nan just added what I think are the only changes needed for Mongo, but I'm not able to test tonight (don't have a quick repo to test against and I'm calling it for the night). Also I made some pretty substantial changes to the logging docs because I felt they were a little all over the place after adding more so I just reorganized. Hope you like it; if not, let me know and I can revert back to the old structure but with the new content. |
Co-authored-by: Martin Adámek <banan23@gmail.com>
|
Also re: tests, looks like they're all failing due to this error: But I didn't change anything related to the CLI 🤔 thoughts? |
|
Test are passing locally again! Should be ready for that through review once you have time 🙂 no rush though! |
Co-authored-by: Martin Adámek <banan23@gmail.com>
Co-authored-by: Martin Adámek <banan23@gmail.com>
…ger to differentiate
Fixes #4230