bugfix#2036 Websocket connections ignore userAgent setting #2151

cooliscool · 2021-10-01T07:27:38Z

For fixing the Bug #2036

Changes I have made:

Initializing header variable beforehand to apply the default User-Agent string (example - "k6/0.27.0 (https://k6.io/)") , OR to apply the User-Agent supplied through Options if it exists.

CLAassistant · 2021-10-01T07:27:41Z

All committers have signed the CLA.

codebien

Hi @cooliscool,
thanks for your contribution! 🎉

Can you also add a small test for asserting the expected logic with the header assigned, please?

codebien · 2021-10-01T08:30:38Z

js/modules/k6/ws/ws.go

+	header := make(http.Header)
 	tags := state.CloneTags()

+	header.Set("User-Agent", state.Options.UserAgent.String)


Can you move this before CloneTags, please? I would prefer to not have it in the middle between initialization and assignment.

Sure.! Working on that.

codecov-commenter · 2021-10-01T10:25:03Z

Codecov Report

Merging #2151 (6a94f66) into master (42f2e60) will increase coverage by 0.01%.
The diff coverage is 71.42%.

❗ Current head 6a94f66 differs from pull request most recent head 243a3ca. Consider uploading reports for the commit 243a3ca to get more accurate results

@@            Coverage Diff             @@
##           master    #2151      +/-   ##
==========================================
+ Coverage   72.71%   72.73%   +0.01%     
==========================================
  Files         184      184              
  Lines       14571    14577       +6     
==========================================
+ Hits        10596    10603       +7     
+ Misses       3333     3331       -2     
- Partials      642      643       +1

Flag	Coverage Δ
ubuntu	`72.66% <71.42%> (+0.01%)`	⬆️
windows	`72.39% <71.42%> (+0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
cmd/login_cloud.go	`0.00% <0.00%> (ø)`
cmd/login_influxdb.go	`0.00% <0.00%> (ø)`
cmd/ui.go	`20.61% <0.00%> (ø)`
lib/metrics/metrics.go	`100.00% <ø> (ø)`
ui/form_fields.go	`45.71% <0.00%> (ø)`
js/modules/k6/ws/ws.go	`83.51% <100.00%> (+0.35%)`	⬆️
output/csv/config.go	`74.00% <100.00%> (+4.23%)`	⬆️
output/csv/output.go	`67.30% <100.00%> (ø)`
output/influxdb/config.go	`41.81% <0.00%> (-1.82%)`	⬇️
core/engine.go	`85.47% <0.00%> (+0.85%)`	⬆️

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 b068f66...243a3ca. Read the comment docs.

mstoykov

LGTM but I would prefer we have a test(or to expand one) to actually check that it works so we don't break it later

cooliscool · 2021-10-05T13:01:02Z

@codebien @mstoykov

Explanation for the changes made:

I couldn't find a way to retrieve headers of a websocket request that got fired, from within JS environment. ( the actual JS WebSockets API doesn't even have an option to set additional headers. refer this stackoverflow question )
So, I've modified the echo server in httpmultibin.go to send back a Echo-User-Agent header in the response if the User-Agent is set by client. In future this could be expanded add tests for any additional headers sent through k6.

Also kindly let me know if there's a better way to implement this test. I will be happy to make it better.

cooliscool · 2021-10-05T17:51:05Z

I can't figure out why lint is breaking with the message
js/modules/k6/ws/ws_test.go:322:2 paralleltest Function TestSession has missing the call to method parallel in the test run

here:

The same pattern as other sub tests in TestSession is followed in the newly added useragent test.
Tests are running fine.
Putting in a t.Parallel() within t.Run() causes the tests to fail.

lib/testutils/httpmultibin/httpmultibin.go

mstoykov · 2021-10-06T15:21:57Z

@cooliscool that is because you are adding a test and the linter is now noticing it, while it's not going to complain about old stuff as otherwise it will be complaining constantly :(. We do in general fix linter errors when they start bubbling up, but I think this might be a bit too much.

On the other hand given that I prefer for you to have a custom handler you can just as well move the whole new test in a new Test function instead of creating a subtest for it as well, and add t.Parallel() only for your test :)

…roved

lib/testutils/httpmultibin/httpmultibin.go

mstoykov

Apart from moving the changes from httpmultibin to the test that uses them I have no other requests and I think after that we can merge this 🎉

Thanks a lot for all the work 🙇

… modifications to httpmultibin

…int issues

cooliscool · 2021-10-11T07:58:18Z

Apart from moving the changes from httpmultibin to the test that uses them I have no other requests and I think after that we can merge this tada

Thanks a lot for all the work bow

Thank you!
I have removed modifications made to httpmultibin, and moved test specific code to TestUserAgent.

mstoykov

LGTM, thanks a lot for all the work 🙇

codebien

LGTM 👍

inancgumus

LGTM and I have tiny suggestions.

js/modules/k6/ws/ws_test.go

…re error logging, minor refactoring

inancgumus

Thanks, I have some minor suggestions.

js/modules/k6/ws/ws_test.go

inancgumus

LGTM, thx!

cooliscool · 2021-10-12T10:38:56Z

I'd like to thank you all for the quick coordination, code review and suggestions. It was a great deal of learning for me.

bugfix#2036 userAgent in WebSocket

a9afa8e

codebien requested changes Oct 1, 2021

View reviewed changes

minor refactoring

b2be0d3

mstoykov reviewed Oct 5, 2021

View reviewed changes

added test for websocket useragent; tests fix for bug#2036

bb44852

cooliscool requested a review from codebien October 5, 2021 12:30

fix lint issue

8f3e7fb

cooliscool requested a review from mstoykov October 5, 2021 17:52

mstoykov reviewed Oct 6, 2021

View reviewed changes

lib/testutils/httpmultibin/httpmultibin.go Outdated Show resolved Hide resolved

mstoykov added this to the v0.35.0 milestone Oct 6, 2021

bugfix#2036 Websocket connections ignore userAgent setting - test imp…

e821be5

…roved

cooliscool requested a review from mstoykov October 6, 2021 18:53

mstoykov reviewed Oct 7, 2021

View reviewed changes

lib/testutils/httpmultibin/httpmultibin.go Outdated Show resolved Hide resolved

mstoykov requested changes Oct 7, 2021

View reviewed changes

cooliscool added 2 commits October 11, 2021 13:07

bugfix#2036 Websocket connections ignore userAgent setting - removing…

ad3b892

… modifications to httpmultibin

bugfix#2036 Websocket connections ignore userAgent setting - fixing l…

b801cb7

…int issues

Merge branch 'grafana:master' into bugfix#2036

6a94f66

cooliscool requested a review from mstoykov October 11, 2021 08:24

mstoykov previously approved these changes Oct 11, 2021

View reviewed changes

codebien previously approved these changes Oct 11, 2021

View reviewed changes

inancgumus reviewed Oct 11, 2021

View reviewed changes

js/modules/k6/ws/ws_test.go Show resolved Hide resolved

js/modules/k6/ws/ws_test.go Outdated Show resolved Hide resolved

cooliscool added 2 commits October 12, 2021 13:11

Merge branch 'grafana:master' into bugfix#2036

243a3ca

bugfix#2036 Websocket connections ignore userAgent setting - added mo…

6826fc8

…re error logging, minor refactoring

cooliscool dismissed codebien’s stale review via 6826fc8 October 12, 2021 07:56

cooliscool dismissed mstoykov’s stale review via 6826fc8 October 12, 2021 07:56

cooliscool requested review from inancgumus, codebien and mstoykov October 12, 2021 07:59

inancgumus requested changes Oct 12, 2021

View reviewed changes

js/modules/k6/ws/ws_test.go Outdated Show resolved Hide resolved

js/modules/k6/ws/ws_test.go Outdated Show resolved Hide resolved

bugfix#2036 improved error messages

13733bc

cooliscool requested a review from inancgumus October 12, 2021 08:47

inancgumus approved these changes Oct 12, 2021

View reviewed changes

codebien approved these changes Oct 12, 2021

View reviewed changes

na-- approved these changes Oct 12, 2021

View reviewed changes

na-- merged commit 055f794 into grafana:master Oct 12, 2021

na-- mentioned this pull request Oct 12, 2021

Websocket connections ignore userAgent setting #2036

Closed

ppcano mentioned this pull request Oct 14, 2021

Added WebSocket specific Params documentation grafana/k6-docs#442

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bugfix#2036 Websocket connections ignore userAgent setting #2151

bugfix#2036 Websocket connections ignore userAgent setting #2151

cooliscool commented Oct 1, 2021

CLAassistant commented Oct 1, 2021 •

edited

Loading

codebien left a comment

codebien Oct 1, 2021

cooliscool Oct 1, 2021

codecov-commenter commented Oct 1, 2021 •

edited

Loading

mstoykov left a comment

cooliscool commented Oct 5, 2021 •

edited

Loading

cooliscool commented Oct 5, 2021

mstoykov commented Oct 6, 2021

mstoykov left a comment

cooliscool commented Oct 11, 2021

mstoykov left a comment

codebien left a comment

inancgumus left a comment

inancgumus left a comment

inancgumus left a comment

cooliscool commented Oct 12, 2021

bugfix#2036 Websocket connections ignore userAgent setting #2151

bugfix#2036 Websocket connections ignore userAgent setting #2151

Conversation

cooliscool commented Oct 1, 2021

For fixing the Bug #2036

Changes I have made:

CLAassistant commented Oct 1, 2021 • edited Loading

codebien left a comment

Choose a reason for hiding this comment

codebien Oct 1, 2021

Choose a reason for hiding this comment

cooliscool Oct 1, 2021

Choose a reason for hiding this comment

codecov-commenter commented Oct 1, 2021 • edited Loading

Codecov Report

mstoykov left a comment

Choose a reason for hiding this comment

cooliscool commented Oct 5, 2021 • edited Loading

Explanation for the changes made:

cooliscool commented Oct 5, 2021

mstoykov commented Oct 6, 2021

mstoykov left a comment

Choose a reason for hiding this comment

cooliscool commented Oct 11, 2021

mstoykov left a comment

Choose a reason for hiding this comment

codebien left a comment

Choose a reason for hiding this comment

inancgumus left a comment

Choose a reason for hiding this comment

inancgumus left a comment

Choose a reason for hiding this comment

inancgumus left a comment

Choose a reason for hiding this comment

cooliscool commented Oct 12, 2021

CLAassistant commented Oct 1, 2021 •

edited

Loading

codecov-commenter commented Oct 1, 2021 •

edited

Loading

cooliscool commented Oct 5, 2021 •

edited

Loading