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: contextual information about processing for loging using MDC #642

Merged
merged 2 commits into from
Nov 2, 2021

Conversation

csviri
Copy link
Collaborator

@csviri csviri commented Oct 29, 2021

closes #37

@csviri csviri linked an issue Oct 29, 2021 that may be closed by this pull request
@metacosm
Copy link
Collaborator

This looks OK to me but I'm wondering how well it plays with Quarkus.

@csviri
Copy link
Collaborator Author

csviri commented Oct 29, 2021

No idea, next week will take some time and dig deep into quarkus side of things.

@csviri
Copy link
Collaborator Author

csviri commented Oct 29, 2021

Maybe we can merge and see, this how is works its just using ThreadLocals usually in background in logging frameworks. So I guess should not be a big deal.

@csviri
Copy link
Collaborator Author

csviri commented Oct 29, 2021

Maybe we can add this now, in worst case we create a revert commit, if there will be huge issues.

@metacosm
Copy link
Collaborator

Another thing is that I feel it's kind of redundant with the monitoring support. Granted, it's probably easier to use than seeing metrics and doesn't quite cover the same use case…

@csviri
Copy link
Collaborator Author

csviri commented Oct 29, 2021

Yes, this is related to log aggregation not to metrics. Like if you have logs in ELK stack and you want to see all logs related to a namespace or a resource. You can easily filter all the related logs.

@csviri
Copy link
Collaborator Author

csviri commented Oct 29, 2021

Actually will investigate how to have better correlation id support in a separate PR. To even better support log aggregation.

@lburgazzoli
Copy link
Collaborator

LGTM, however I wonder if we should make such things something a user can opt in

@csviri
Copy link
Collaborator Author

csviri commented Nov 2, 2021

LGTM, however I wonder if we should make such things something a user can opt in

Hmm good question, so if the user should be able to specify the keys and values interested in. Wondering what would be the scope of that, just the custom resource?

Doing the refactor on the other branch, so would rather merge this and create a separate issue and see if there is an elegant way to do it that way.

@csviri csviri merged commit 5d23dca into v2 Nov 2, 2021
@csviri csviri deleted the mdc-support branch November 2, 2021 10:07
@lburgazzoli
Copy link
Collaborator

LGTM, however I wonder if we should make such things something a user can opt in

Hmm good question, so if the user should be able to specify the keys and values interested in. Wondering what would be the scope of that, just the custom resource?

I meant more a flag to enable/disable MDC entirely (or a dedicated module that enable MDC logging if on the classpath)

@csviri
Copy link
Collaborator Author

csviri commented Nov 2, 2021

@lburgazzoli what would be the benefit of that?
You configure the logging pattern not to log it. SLF4J contains it by default.

@lburgazzoli
Copy link
Collaborator

There is usually a cost associated MDC (thread local, maps and other allocations) so if you don't require MDC, then you can turn it off and save some memory

@csviri
Copy link
Collaborator Author

csviri commented Nov 2, 2021

yes, but that part usually doable on the impl level,
for log4j2 at least (on the bottom) https://logging.apache.org/log4j/2.x/manual/thread-context.html

So not sure if it makes that much sense to solve on this level. In case the logger implementation would not handle that sure.

@lburgazzoli
Copy link
Collaborator

I don't have any strong opinion, so it is fine with me but as personal preference, I always like more when I have the option to opt in to something that to opt out :)

@csviri
Copy link
Collaborator Author

csviri commented Nov 3, 2021

I don't have any strong opinion, so it is fine with me but as personal preference, I always like more when I have the option to opt in to something that to opt out :)

A completely agree in general. Want to do a round to review for v2 how we are configuring the operator and controller. Will take in mind also this and put it under a feature flag if doable nicely.

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.

Using MDC (Mapped Diagnostic Context) to improve logging
3 participants