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 tagged logging support #35

Closed

Conversation

markglenn
Copy link
Contributor

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 #8

Implement tagged logging similar to ActiveSupport::TaggedLogging.
Don't use the default_options to pass the tags into the logger.  This
causes the tags field to be sent over to graylog as an array.  Instead
pass the tag names into an attribute called log_tags that matches with
the Rails standard of config.log_tags.
@markglenn markglenn changed the title Feature/tagged logging Add tagged logging support May 8, 2015
@mattcg
Copy link

mattcg commented Apr 23, 2016

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

@mattcg
Copy link

mattcg commented May 3, 2016

@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
Copy link
Contributor

milgner commented Aug 21, 2016

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?

end

def current_tags
Thread.current[:gelf_tagged_logging_tags] ||= []
Copy link
Contributor

Choose a reason for hiding this comment

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

In my experience it's slightly safer to use thread_variable_set / thread_variable_get.

@jalogisch
Copy link

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

@milgner
Copy link
Contributor

milgner commented Mar 18, 2018

@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@73e3838 - not sure what to do about the CLA though - do you know whether it's still required?

@jalogisch
Copy link

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

@jalogisch
Copy link

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

@markglenn
Copy link
Contributor Author

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

@markglenn
Copy link
Contributor Author

markglenn commented Mar 27, 2018

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
Copy link

Meat-Chopper commented Mar 26, 2019

Solved merge conflicts in Meat-Chopper@1f799ca
Just in case the new PR #81 has been created .

@CLAassistant
Copy link

CLAassistant commented Sep 17, 2019

CLA assistant check
All committers have signed the CLA.

@markglenn
Copy link
Contributor Author

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.

@markglenn markglenn closed this Sep 17, 2019
@mattcg
Copy link

mattcg commented Sep 17, 2019

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants