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

Add support to hide metadata in log line #445

Closed
wants to merge 10 commits into from

Conversation

timlohse1104
Copy link
Contributor

See #444

With option --hideMetadata it is possible to transform a basic log line like this:

[25.7.2023, 15:57:32] INFO: [Nest] 25.7.2023, 15:57:32 [RoutesResolver] | OcrController {/ocr}: {"context":"RoutesResolver"}

into a simulated NestJS log line like this (provided you use the messageFormat option):

[Nest] 25.7.2023, 15:57:32 [RoutesResolver] | OcrController {/ocr}: {"context":"RoutesResolver"}

--hideMetadata option effectively hides epoch, leveland other metadata like reqId from beeing printed in front of the actual log message.

@timlohse1104 timlohse1104 changed the title Add support to hide metadata in log line Add support to hide metadata in log line #444 Jul 25, 2023
@timlohse1104 timlohse1104 changed the title Add support to hide metadata in log line #444 Add support to hide metadata in log line Jul 25, 2023
@timlohse1104
Copy link
Contributor Author

@jsumners or @mcollina What do you think?
Would love to provide you with further pull requests.

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

} else if (prettifiedTime) {
line = `${line} ${prettifiedTime}`
}
if (!opts.hideMetadata) {
Copy link
Member

Choose a reason for hiding this comment

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

Can this be done in some other way than continually nesting the core code of this function further and further to the right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you have something in mind. I could see some options. One with higher refactoring effort like using strategy pattern (https://refactoring.guru/design-patterns/strategy) and some with lower effort like using switch statements/ lookup tables.

Copy link
Member

@jsumners jsumners Jul 30, 2023

Choose a reason for hiding this comment

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

I think a simple approach would be to move the now optional code path into a function. Note, such a function should return a string, not mutate one through a side effect.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added an approach

@@ -206,6 +178,46 @@ function prettyFactory (options) {
}

return line

function createLineMetadata () {
Copy link
Member

Choose a reason for hiding this comment

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

Does this function need to be defined as an inner function of prettyFactory? Or can we put it in lib/utils?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could put it in lib/utils but would have to pass many parameters like log, colorizer, levelKey etc. Think i would prefer to leave it in index.js. What do you think about the naming?

Copy link
Member

Choose a reason for hiding this comment

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

My concern is legibility and maintainability. The inlined function is easier to read than the nested conditional version, but it is still a lot of code deeply nested within another function.

I think the function name is fine.

Copy link
Member

Choose a reason for hiding this comment

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

@timlohse1104 I think you'll find it much easier to accommodate the ask now that #453 has been merged. Unfortunately, it'll be easier to start with a fresh PR. Rebasing this one may be too difficult.

If you have any questions, don't hesitate to ask.

@jsumners
Copy link
Member

I'm closing this due to lack of response. If you'd like to finish this work, please open a new PR.

@jsumners jsumners closed this Sep 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants