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

Implemented AnonymizeQueryStringValues option (proposal 1 from #927). #1171

Conversation

marklagendijk
Copy link
Contributor

@marklagendijk marklagendijk commented Sep 23, 2019

This pull requests implements proposal 1 from #927.
The AnonymizeQueryStringValues option is added, which when enabled anonymizes the query string values of the url sent to the name tag.
Example: https://my-url?name=value&name2=value2 => https://my-url?name=[]&name2=[]

@CLAassistant
Copy link

CLAassistant commented Sep 23, 2019

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@codecov-io
Copy link

codecov-io commented Sep 23, 2019

Codecov Report

Merging #1171 into master will decrease coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #1171      +/-   ##
=========================================
- Coverage   73.61%   73.6%   -0.01%     
=========================================
  Files         144     144              
  Lines       10529   10534       +5     
=========================================
+ Hits         7751    7754       +3     
- Misses       2321    2322       +1     
- Partials      457     458       +1
Impacted Files Coverage Δ
lib/netext/httpext/request.go 97.09% <100%> (+0.05%) ⬆️
lib/options.go 92.89% <100%> (+0.07%) ⬆️
core/engine.go 92.99% <0%> (-0.94%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ab970ca...5207e7a. Read the comment docs.

js/modules/k6/http/request_test.go Outdated Show resolved Hide resolved
js/modules/k6/http/request_test.go Outdated Show resolved Hide resolved
lib/netext/httpext/request.go Outdated Show resolved Hide resolved
@na--
Copy link
Member

na-- commented Sep 24, 2019

@marklagendijk , thanks for the PR! I quickly skimmed it just now and while I didn't spot any big issues with the code, but I'm sorry to say that we might delay actually merging this until we plan out some future k6 changes, or potentially not merge the PR at all 😞 . In particular, we recently decided that we'll create a brand new JS HTTP API to address all of the shortcomings with the current k6/http API that we can't fix in a backwards compatible manner. One of these shortcomings is the inability to alter metric tags, like we discussed in your original issue.

But whereas your PR adds a new global config option (which we are reluctant to do because of #883) for solving a part of that issue in a very specific way, we'd like to have something more flexible and unobtrusive. For example, here are some things that the current approach doesn't solve:

  • I may not want to overwrite the query values for all HTTP requests in my script, only for some
  • I may also need to remove the query parameter names, not just the values
  • I may need to replace the values with something other than []

We still don't have a proper GitHub issue describing the new HTTP API that we'd like to build, since we only just had a long verbal discussion about how it should look like last week. When we create one, we'll link this PR and the original issue that prompted it. But as a sneak peek, one of the things we want in the new API is to have a user-customizable HTTP Client object, somewhat similar to the Go stdlib's http.Client. That way users can set things like default options, header values, cookies, metric tags, etc. for their requests (and yes, we would be able to have multiple different clients in the same script), but we also want to expose a callback method that allows them to modify things like the emitted metric tags for any requests.

As I said, we still don't have the new API fully mapped out, but we're pretty sure that we would be able to fit all of these features in a very developer-friendly and natural way. Thanks again for the PR and sorry that we have to delay or not merge it, but please stay tuned for the new HTTP API, I'm hoping that you'd like it, since it should solve a lot of the issues with the current one, without actually being any more complicated to use at all for simple things.

@na-- na-- added the evaluation needed proposal needs to be validated or tested before fully implementing it in k6 label Sep 24, 2019
@mstoykov
Copy link
Contributor

I did some quick benchmarking in order to see if a naive for loop will be much faster and .... it is:

$ go test -v -bench=. -benchmem
goos: linux
goarch: amd64
pkg: github.com/loadimpact/k6/b
BenchmarkA-4 1000000 1894 ns/op 576 B/op 15 allocs/op
BenchmarkB-4 300000 5304 ns/op 938 B/op 28 allocs/op
PASS
ok github.com/loadimpact/k6/b 4.257s

But as @na-- pointed out we might not be merging this anytime soon or at all but I would still prefer if you did change to the not regexp version ... maybe try with strings.Index as well in the mean time

@marklagendijk
Copy link
Contributor Author

@na-- although it will take some more time, I do really like the idea of having an improved HTTP module.

A long time ago I created an issue about this #436. There are some concrete suggestions in there, based on the Node.js request library. I would highly recommend taking a good look at request, not only because it is a good library (it is), but also because it is very well known with Javascript developers.

@na-- na-- added the new-http issues that would require (or benefit from) a new HTTP API label Oct 18, 2021
@na--
Copy link
Member

na-- commented Mar 31, 2022

Closing this in favor of #436, #2461, and all of the other new-http issues.

@na-- na-- closed this Mar 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
evaluation needed proposal needs to be validated or tested before fully implementing it in k6 new-http issues that would require (or benefit from) a new HTTP API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants