graylog-labs / gelf-rb

Ruby GELF library (Graylog Extended Log Format)
https://rubygems.org/gems/gelf
MIT License
153 stars 104 forks source link

Add tagged logging support #35

Closed markglenn closed 4 years ago

markglenn commented 9 years ago

This adds support for tagged logging that is compatible with ActiveSupport::TaggedLogging.

I'm open for discussion on whether the initialization for the tagged logging is the right way to go. In a Rails application config file, I would use it like this:

config.logger = GELF::Logger.new('localhost', 12201, 'WAN', { facility: 'my_application' })
config.log_tags = [:uuid]
config.logger.log_tags = [:request_uuid]

From the other pull requests submitted prior to this, they added the tag names in the default_options hash, but this causes that array to be pushed onto Graylog. We could remove that key from default_options, but that could be an issue if someone would like to use that hash key we choose for their own use. It also doesn't make sense to me to put configuration in the default options hash, since this hash is the default hash for every message to Graylog.

Instead I'm opting to create an attribute called log_tags on the LoggerCompatibility module which mimics the Rails method.

If this looks ok, I can add to the README.rdoc file on usage.

Thanks to @mitnal since this is heavily inspired by pull request https://github.com/Graylog2/gelf-rb/pull/8

mattcg commented 8 years ago

Is there a reason why this was never pulled? It would be very useful to have this feature.

mattcg commented 8 years ago

@bernd pinging you because you seem to be the most active committer right now. This feature is important to me because it's the only way to tag error logs with a request UUID that would allow us to trace the error through our entire stack. What's holding it back from being merged? I'm volunteering to contribute whatever might be required :)

milgner commented 8 years ago

Thank you for the contribution, @markglenn! It seems to me that it would be even better to have this in a separate TaggedLogging module which could be mixed in to the regular logger. What you think?

jalogisch commented 6 years ago

@milgner I would close this PR as this is really old - please comment on that within a week or i'll close it.

milgner commented 6 years ago

@jalogisch after additional thought - I wasn't sure about the naming at first - I think this should be merged after all. I made an additional commit to replace Thread#[] with Thread#thread_variable_get/set at milgner/gelf-rb@73e383847ee585f26e08d446e4b282b9733f926a - not sure what to do about the CLA though - do you know whether it's still required?

jalogisch commented 6 years ago

thanks for the comment @milgner - yes the CLA is required. Without we are not able to merge that.

jalogisch commented 6 years ago

@markglenn if you sign the CLA and update the PR we would review and merge.

markglenn commented 6 years ago

@jalogisch I signed the CLA 3 years ago when I first created this pull request (and another one that did get pulled in).

markglenn commented 6 years ago

I also believe this can't be merged in as is without a decent amount of rework due to changes in the code layout. I vote to close this pull request and have someone take a stab at porting it to the newer code layout. If I need to re-sign the CLA to allow someone to do that, I'm happy to sign.

Meat-Chopper commented 5 years ago

Solved merge conflicts in https://github.com/Meat-Chopper/gelf-rb/commit/1f799ca952bd4ea8ce644ec6ebbd861c64e91df1 Just in case the new PR https://github.com/graylog-labs/gelf-rb/pull/81 has been created .

CLAassistant commented 4 years ago

CLA assistant check
All committers have signed the CLA.

markglenn commented 4 years ago

Closing this in lieu of PR #81

Good luck @Meat-Chopper in getting yours merged in. I'd like this in master so we can get off our forked copy. Honestly, their management of the CLA is frustrating.

mattcg commented 4 years ago

Wow, what a mess. Seriously, Graylog people. I love your product but if you want help from the OS community you have to take this a bit more seriously. Next time no one is going to bother.