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

Allow filtering params based on parent keys #13897

Merged
merged 1 commit into from
Jul 6, 2015

Conversation

gmalette
Copy link
Contributor

Some parameters on APIs have ambiguous names
that cannot be filtered altogether.

For example, it makes sense not to filter :code in

{ file: { code: '<% source %>' }}

It makes sense however to filter it in

{ credit_card: { code: '424242424242' }}

This PR allows doing so by using

config.filter_parameters += ["credit_card.code"]

@gmalette
Copy link
Contributor Author

This is an alternative take on #13670

@gmalette
Copy link
Contributor Author

gmalette commented Feb 1, 2014

@tenderlove, you're the last one to have worked on this. Any thoughts?

@arthurnn
Copy link
Member

arthurnn commented Mar 4, 2014

👍

@tenderlove
Copy link
Member

param_filter is a hotspot for perf which is why I took care to remove as much logic from call as possible. Unless there is an extremely good reason for this change, I'm 👎.

@gmalette
Copy link
Contributor Author

gmalette commented Mar 4, 2014

I'll benchmark it.

@gmalette
Copy link
Contributor Author

gmalette commented Mar 5, 2014

You were right that it was much slower. The updated patch is a bit larger, but the performances are roughly the same as master.

The params have synthetic depth of N.

2.1.0
1x10 (50000)
                      user     system      total        real
vanilla           4.650000   0.010000   4.660000 (  4.671173)
modified no .     4.620000   0.000000   4.620000 (  4.626721)
modified with .   5.940000   0.000000   5.940000 (  5.947298)


3x10 (5000)
                      user     system      total        real
vanilla          47.190000   0.010000  47.200000 ( 47.302018)
modified no .    48.530000   0.010000  48.540000 ( 48.643803)
modified with .  60.900000   0.010000  60.910000 ( 61.044795)


5x10 (1000)
                      user     system      total        real
vanilla          989.440000   0.710000  990.150000 ( 992.263921)
modified no .   1002.170000   0.050000 1002.220000 (1004.351774)
modified with . 1305.360000   0.910000 1306.270000 (1309.029364)

here's the script for the benchmark: https://gist.github.com/gmalette/9367909

@rafaelfranca rafaelfranca modified the milestones: 4.2.0, 5.0.0 Aug 18, 2014
@gmalette
Copy link
Contributor Author

This has been in production for a while. It slowed the log filtering time by 20-30% depending on where.

Our average is now sitting at 0.12ms when it was previously taking 0.10ms

7fe55404-23f2-11e4-9bc8-f80c17a4f5d5

@gmalette
Copy link
Contributor Author

gmalette commented Apr 2, 2015

Has any thought been given to this? We've been running it for a while now

@rafaelfranca
Copy link
Member

cc @tenderlove @jeremy

@arthurnn
Copy link
Member

What is the slowest part of the new code? is this going to effect all apps, or only the ones that use this new feature?

@gmalette gmalette force-pushed the nested-parameter-filtering-2 branch from df7f383 to b067568 Compare June 12, 2015 18:23
@gmalette
Copy link
Contributor Author

What is the slowest part of the new code?

The slowest part of the new code is the line that assembles the new key and runs the regex. People not using the feature will not be impacted. The slowdown for people not using the feature comes from the management of parent keys for each new level of nesting. As shown in the current benchmarks, the impact is minimal.

Here are benchmarks for Ruby 2.2.1 (same code, but using IPS)

SMALL
Calculating -------------------------------------
     vanilla           844.000  i/100ms
     modified no .     725.000  i/100ms
     modified with .   658.000  i/100ms
-------------------------------------------------
     vanilla              8.838k (± 2.6%) i/s -     44.732k
     modified no .        8.705k (± 3.0%) i/s -     44.225k
     modified with .      7.016k (± 2.9%) i/s -     35.532k


MEDIUM
Calculating -------------------------------------
     vanilla             8.000  i/100ms
     modified no .       8.000  i/100ms
     modified with .     7.000  i/100ms
-------------------------------------------------
     vanilla             86.996  (± 3.4%) i/s -    440.000
     modified no .       85.721  (± 3.5%) i/s -    432.000
     modified with .     70.258  (± 4.3%) i/s -    357.000

LARGE
Calculating -------------------------------------
     vanilla             1.000  i/100ms
     modified no .       1.000  i/100ms
     modified with .     1.000  i/100ms
-------------------------------------------------
     vanilla              0.863  (± 0.0%) i/s -      5.000  in   5.795388s
     modified no .        0.849  (± 0.0%) i/s -      5.000  in   5.888606s
     modified with .      0.651  (± 0.0%) i/s -      4.000  in   6.169669s

@arthurnn
Copy link
Member

@gmalette Sounds good to me. Can you add a changelog before I merge it?
thanks

Add the possibility to only filter parameters based on
their full path instead of relying on the immediate key.

    config.filter_parameters += ['credit_card.code']

    { 'credit_card' => { 'code' => '[FILTERED]' },
      'source' => { 'code' => '<%= puts 5 %>' } }
@gmalette gmalette force-pushed the nested-parameter-filtering-2 branch from b067568 to 33b9317 Compare June 22, 2015 14:04
@gmalette
Copy link
Contributor Author

updated

arthurnn pushed a commit that referenced this pull request Jul 6, 2015
Allow filtering params based on parent keys
@arthurnn arthurnn merged commit e598967 into rails:master Jul 6, 2015
@arthurnn
Copy link
Member

arthurnn commented Jul 6, 2015

Merci beaucoup .

@rafaelfranca rafaelfranca modified the milestones: 5.0.0 [temp], 5.0.0 Dec 30, 2015
bdewater added a commit to bdewater/rails that referenced this pull request Feb 29, 2016
sgrif added a commit that referenced this pull request Feb 29, 2016
Add documentation for #13897 [skip ci]
@gmalette gmalette deleted the nested-parameter-filtering-2 branch May 18, 2017 17:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants