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

context support #44

Closed
wants to merge 1 commit into from
Closed

context support #44

wants to merge 1 commit into from

Conversation

prepor
Copy link

@prepor prepor commented Dec 4, 2013

Just suggestion for #42

@prepor prepor restored the context-support branch December 4, 2013 16:17
@ptaoussanis
Copy link
Member

Hi Andrew, thanks for this!

I may be misunderstanding the intention a little - what would we gain from making this a Timbre feature over, say, just doing this on the application-side?

Remember that appenders are just fns, so you could write an appender that checks+uses the state of a dynamic variable (like *context* here). Since appenders see raw arguments, it'd also be possible to provide context via logging arguments themselves: (timbre/infof "User logged in: %s" user-name {:user-id user-id}).

In that last example, the appender could be written to check appender args for a map with a :user-id key. (Or :context or whatever). You can see an example of this technique here: https://github.com/ptaoussanis/timbre/blob/master/src/taoensso/timbre.clj#L193 where some appenders will look for a map with a :timbre/hash key to decide whether two log entries should be considered "the same".

Let me know what you think, cheers! :-)

@prepor
Copy link
Author

prepor commented Dec 5, 2013

Anything can be done on application side :) In Timbre it can be "specification" for appenders. And appenders can handle this in their way as doing so in log4j-world with MDC and NDC. Contexts can be used in standard redis appender, and in future standard graylog and splunk appenders.

@ptaoussanis
Copy link
Member

Hi Andrew, okay - I'd like a little time to think about this & possible alternatives. Your PR definitely helped clarify your objective, so thanks for that!

Will get back to you.

Cheers :-)

@prepor
Copy link
Author

prepor commented Dec 6, 2013

Thank you!

@prepor prepor closed this Dec 6, 2013
@hugoduncan
Copy link
Collaborator

FWIW, we ended up implementing something very similar, using middleware to add the value of a *context* var onto a :context key. https://github.com/palletops/log-config/blob/develop/src/com/palletops/log_config/timbre.clj#L49

@ptaoussanis
Copy link
Member

Hi Hugo, looks good! Thanks for pinging about this.

Your library looks interesting too! Shall I perhaps link to it from the Timbre README, or is it something you're just using internally?

@hugoduncan
Copy link
Collaborator

I'ld be honoured if you linked it. At the moment we're just using this internally, but other users (and contributors) are definitely welcome. Also more than happy should any of the functionality end up in Timbre itself.

@ptaoussanis
Copy link
Member

Okay great, just added a link :-) Haven't had a chance yet to look at it in detail, but would certainly be open to PRs that you think might be useful or better-integrated upstream.

Have a great weekend, cheers!

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