Only log controller parameters when configured to. #20
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This adds a new config option `log_controller_parameters`, that defaults to false. This needs to be set to true to enable logging of the params hash, otherwise this is omitted. Rationale: The change in #14 has implications that are not immediately obvious. It will cause the elasticsearch mapping for the parameters field to become a new dynamic mapping that gets indexed. This can lead to collisions if different apps use the same params key with different types, which results in log messages being lost. It also causes the indexes to become significantly larger (in our case, this change tripled the size). In light of this, I think the not logging these fields by default is the correct choice.
|
Thanks for this. We have some actions with quite a few parameters, and our log size simply exploded. I'd like to see this merged into master. |
alext
added a commit
to alphagov/errbit
that referenced
this pull request
Jan 27, 2014
Using our branch until shadabahmed/logstasher#20 is merged
|
Would definitely be more than useful, as new settings often make logged requests huge. |
|
Looks like i can't find |
|
Missing a trailing 'g' - 'disable_parameter_logging' |
shadabahmed
added a commit
that referenced
this pull request
Jan 31, 2014
|
Oopsies .. done and released .. |
|
Hi, unless I'm missing something, the change didn't get merged in, only the version bumped... |
shadabahmed
added a commit
that referenced
this pull request
Jan 31, 2014
shadabahmed
added a commit
that referenced
this pull request
Jan 31, 2014
|
I am totally lost today - apologies - all merged now in 0.4.8 |
|
No worries, I know the feeling... Thanks for merging. |
alext
added a commit
to alphagov/errbit
that referenced
this pull request
Feb 3, 2014
Now that shadabahmed/logstasher#20 has been merged
alext
added a commit
to alphagov/asset-manager
that referenced
this pull request
Feb 17, 2014
alext
added a commit
to alphagov/places-manager
that referenced
this pull request
Feb 17, 2014
alext
added a commit
to alphagov/publisher
that referenced
this pull request
Feb 17, 2014
alext
added a commit
to alphagov/whitehall
that referenced
this pull request
Feb 17, 2014
alext
added a commit
to alphagov/panopticon
that referenced
this pull request
Feb 17, 2014
alext
added a commit
to alphagov/business-support-api
that referenced
this pull request
Feb 17, 2014
alext
added a commit
to alphagov/maslow
that referenced
this pull request
Feb 17, 2014
This was referenced Feb 17, 2014
alext
added a commit
to alphagov/release
that referenced
this pull request
Feb 17, 2014
alext
added a commit
to alphagov/router-api
that referenced
this pull request
Feb 17, 2014
alext
added a commit
to alphagov/smart-answers
that referenced
this pull request
Feb 17, 2014
alext
added a commit
to alphagov/static
that referenced
this pull request
Feb 17, 2014
alext
added a commit
to alphagov/support
that referenced
this pull request
Feb 17, 2014
This was referenced Feb 17, 2014
alext
added a commit
to alphagov/trade-tariff-admin
that referenced
this pull request
Feb 17, 2014
alext
added a commit
to alphagov/trade-tariff-frontend
that referenced
this pull request
Feb 17, 2014
alext
added a commit
to alphagov/trade-tariff-backend
that referenced
this pull request
Feb 17, 2014
This was referenced Feb 17, 2014
alext
added a commit
to alphagov/transaction-wrappers
that referenced
this pull request
Feb 17, 2014
alext
added a commit
to alphagov/transition
that referenced
this pull request
Feb 17, 2014
This was referenced Feb 17, 2014
alext
added a commit
to alphagov/travel-advice-publisher
that referenced
this pull request
Feb 17, 2014
alext
added a commit
to alphagov/fco-services
that referenced
this pull request
Feb 17, 2014
This was referenced Feb 17, 2014
alext
added a commit
to alphagov/whitehall
that referenced
this pull request
Mar 10, 2014
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This adds a new config option
log_controller_parameters, that defaults to false. This needs to be set to true to enable logging of the params hash, otherwise this is omitted.Rationale:
The change in #14 has implications that are not immediately obvious. It will cause the elasticsearch mapping for the parameters field to become a new dynamic mapping that gets indexed. This can lead to collisions if different apps (or even different controllers within the same app) use the same params key with different types, which results in log messages being lost. It also causes the indexes to become significantly larger (in our case, this change tripled the size).
In light of this, I think the not logging these fields by default is the correct choice.