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

Adding filtering to the logger #5038

Merged
merged 7 commits into from
Oct 9, 2015

Conversation

simianhacker
Copy link
Member

- Closes elastic#5036
- Add `applyFilterToKey()`
- Add test for `applyFilterToKey()`
- Add `filter` attribute to config for reporters
- Add `this.filter` method to `LogFormat` class
dest: config.get('logging.dest')
dest: config.get('logging.dest'),
filter: {
authorization: 'remove'
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this just remove the "authorization" key from every object logged?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes


it('should remove an entire branch', function () {
var data = fixture();
applyFilterToKey(data, 'headers', 'remove');
Copy link
Contributor

Choose a reason for hiding this comment

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

This breaks when you use "censor".

@spalger spalger assigned simianhacker and unassigned spalger Sep 25, 2015
- Add ability to specify filters from CLI options
- Add test and fix for removing branch with censor
- Add change regex to use strings
@simianhacker simianhacker assigned spalger and unassigned simianhacker Sep 26, 2015
@@ -61,7 +61,7 @@ module.exports = Joi.object({

events: Joi.any().default({}),
dest: Joi.string().default('stdout'),

filter: Joi.any().default({}),
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we move the defaults here please? With the current implementation you can't log authorization headers even if you wanted to. Missed the note below

@spalger
Copy link
Contributor

spalger commented Sep 30, 2015

This currently fails when the --verbose flag is passed because some of the log messages include circular structures (causing Maximum call stack size exceeded).

Since we are only interested in filtering values that will end up in the final output, perhaps we should start applyFilterToKey() with a JSON.parse(JSON.stringify(obj)) (preferably outside of the recursion). This changes the semantics of applyFilterToKey() though, as it is no longer modifying its input but creating a new filtered output.

`applyFilterToKey()` used to choke on circular structures by simply recursing through the object passed in. When we create a circular structure though, we generally provide a simplified version of the object via `obj#toJSON()`. This makes logging those objects simple, and now the filter will use the same mechanism before attempting to itterate the object.
@spalger
Copy link
Contributor

spalger commented Sep 30, 2015

I took a stab at fixing this and sent you a PR simianhacker#6

@rashidkpc rashidkpc assigned simianhacker and unassigned spalger Oct 6, 2015
[server/log] simplify the log messages before filtering
return obj;
}

module.exports = function applyFilterToKey(obj, key, action) {
Copy link
Member

Choose a reason for hiding this comment

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

This seems a tad strange to me to have one function exported by module.exports and one as a property (module.exports. applyFiltersToKeys). I think personally it would be better to export just applyFiltersToKeys, seeing as how that's all that is used anyway (besides in the tests), or export both under keys (applyFiltersToKey and applyFiltersToKeys).

@lukasolson
Copy link
Member

Other than the comment above, LGTM.

@lukasolson lukasolson assigned simianhacker and unassigned lukasolson Oct 7, 2015
@simianhacker simianhacker removed their assignment Oct 8, 2015
@lukasolson
Copy link
Member

LGTM.

@lukasolson lukasolson assigned simianhacker and unassigned lukasolson Oct 8, 2015
simianhacker added a commit that referenced this pull request Oct 9, 2015
@simianhacker simianhacker merged commit 3cda9ad into elastic:master Oct 9, 2015
@epixa
Copy link
Contributor

epixa commented Nov 5, 2015

This was never backported to 4.2

epixa added a commit that referenced this pull request Nov 5, 2015
@simianhacker simianhacker deleted the filter-logs-fix branch March 16, 2017 21:31
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.

4 participants