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

feat: add function to add Correlation IDs to logger #37

Merged
merged 2 commits into from
Mar 3, 2022

Conversation

Pitasi
Copy link
Contributor

@Pitasi Pitasi commented Mar 1, 2022

This function helps in preparing a new logger with correlation ID taken from http request context.

@Pitasi Pitasi added the patch label Mar 2, 2022
@Pitasi Pitasi requested a review from sgerogia March 2, 2022 08:56
Copy link
Contributor

@sgerogia sgerogia left a comment

Choose a reason for hiding this comment

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

Approving to unblock, left an open q


correlationID, ok := ctx.Value(CorrelationIDName).(string)
if ok && len(correlationID) > 0 {
l = l.With(string(CorrelationIDName), correlationID)
Copy link
Contributor

Choose a reason for hiding this comment

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

Will this result in a constant GC overhead?

First instance created by With() is not used and is subject to GC.
The above happening for 1000s of requests.

Copy link
Contributor Author

@Pitasi Pitasi Mar 2, 2022

Choose a reason for hiding this comment

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

Are you saying we can combine the two With()s of this function? Probably yes, I can do it.

Also please do note that this is not a middleware so it's not called for each request, atm it will only be called by api-server in case of panics. I hope we won't have 1000s of panics 😂

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I avoided the intermediary logger and added unit tests. It should be fine now 😄

@Pitasi Pitasi force-pushed the feat/add-correlationid-to-logger-util branch from 3b764dd to 1158d92 Compare March 2, 2022 17:50
@Pitasi Pitasi force-pushed the feat/add-correlationid-to-logger-util branch from 1158d92 to f1b20a6 Compare March 3, 2022 10:06
@Pitasi Pitasi merged commit a080465 into main Mar 3, 2022
@Pitasi Pitasi deleted the feat/add-correlationid-to-logger-util branch March 3, 2022 12:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants