-
Notifications
You must be signed in to change notification settings - Fork 137
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
Better log levels in the message publisher #343
Conversation
I pushed a commit to fix the tests on older ruby versions; filter is an alias of select. Please review them independently. |
@@ -184,7 +184,7 @@ def bindings | |||
|
|||
filtered = api_client.bindings. | |||
reject { |b| b['destination'] == b['routing_key'] }. | |||
filter { |b| b['source'] == @config[:mq_exchange] && b['vhost'] == @config[:mq_vhost] } | |||
select { |b| b['source'] == @config[:mq_exchange] && b['vhost'] == @config[:mq_vhost] } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How is this change relevant?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This fixes build failures for older ruby versions, see travis builds: https://travis-ci.org/gocardless/hutch/jobs/652451121?utm_medium=notification&utm_source=github_status
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Understood, thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, should we log publishes at debug level? Publishes can happen at a rate of hundreds or thousands per second, I really doubt it makes much sense to log at info level regardless of message size.
I'm fine with any solution as long as the current verbosity goes down. Just let me know what changes you want me to make. |
@sdwolfz let's keep the changes you have but change publish logging level to |
Are you sure you want to have two |
Good point. I think simply switching to debug logging would be best. |
Having the entire payload logged at the "INFO" level is not ideal due to the following reasons: - PII data might be contained there and there is no way to fitler it out - Payload might be big enough that logging aggregators might struggle with the sheer amount of data - Since the log level is shared by reusing the rails/sinatra logger, INFO level might be too verbose for hutch but WARN level not verbose enough for the other. While initially I considered implementing a feature to allow for filtering the payload for PII or triming the output, a quick solution to all problems listed above is to change the logging statement to DEBUG, which should only be needed in development/testing environments.
@michaelklishin all done! ♻️ 🙏 |
Thank you! |
Cheers mate! Any hint on when this will be released to rubygems? |
We haven't updated release notes in a while so it will take some time. Hopefully in the next few days. |
@michaelklishin Can I help with that? I assume I need to open a PR with changelog lines for all PRs since the last tag. |
Having the entire payload logged at the "INFO" level is not ideal due to
the following reasons:
the sheer amount of data
might be too verbose for hutch but WARN level not verbose enough for the other.
While initially I considered implementing a feature to allow for filtering
the payload for PII or triming the output, a quick solution to all problems
listed above is to split the logging into two, the INFO level handing the
generic part of the log, with the message ID injected for easy tracking
and in case the payload is needed, log level can be set to DEBUG, which
should only be needed in development/testing environments.