Emit iterations metric as part of netext.NetTrail #1203

imiric · 2019-10-14T13:50:57Z

This removes emission of metrics.Iterations as a standalone metric, and instead bundles it as part of netext.NetTrail, and only for default functions.

This is done to reduce the amount of individual metrics, and thus traffic, sent to backends, and has minor user-facing impact mentioned in #1189.

Closes: #1189

core/engine_test.go

js/runner.go

stats/cloud/collector.go

core/engine_test.go

Resolves: #1203 (comment) Because the iterations metric is now emitted before the minIterationDuration sleep is done--possibly what should've been the correct behavior--it changes these test expectations. And since the previous test wasn't checking for more than one minIterationDuration loop, it makes sense to tweak the settings a bit to gather more data.

na--

LGTM in general. As I mentioned in the inline comment, it would definitely be better if we refactor it, but we should probably do that in a separate PR, most likely in #1007 or after it.

js/runner.go

Resolves: #1203 (comment) Because the iterations metric is now emitted before the minIterationDuration sleep is done--possibly what should've been the correct behavior--it changes these test expectations. And since the previous test wasn't checking for more than one minIterationDuration loop, it makes sense to tweak the settings a bit to gather more data.

codecov-io · 2019-10-29T15:39:52Z

Codecov Report

Merging #1203 into master will increase coverage by 0.16%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1203      +/-   ##
==========================================
+ Coverage   75.12%   75.29%   +0.16%     
==========================================
  Files         147      147              
  Lines       10709    10709              
==========================================
+ Hits         8045     8063      +18     
+ Misses       2199     2181      -18     
  Partials      465      465

Impacted Files	Coverage Δ
core/local/local.go	`91.25% <ø> (-0.24%)`	⬇️
stats/cloud/collector.go	`77.6% <100%> (+4.99%)`	⬆️
js/runner.go	`82.1% <100%> (ø)`	⬆️
lib/netext/dialer.go	`97.43% <100%> (+3.06%)`	⬆️

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 9554c6e...9cd26e4. Read the comment docs.

Resolves: #1203 (comment)

imiric · 2019-10-29T16:03:02Z

I fixed the test issues, but it involved some test refactoring, so let me know if everything makes sense there, or if additional tests are needed for this.

core/engine_test.go

mstoykov

LGTM

na--

As I mentioned in the inline comment, I feel like we need a small test that confirms that iterations aren't emitted for actually incomplete iterations.

Resolves: #1203 (comment)

na--

Ah, can you put the sleep(0.15); after the testCounter.add(1); - the reason it was done that way is that the original test actually tested that for incomplete iterations, metrics that were reached, like the testcounter, will still be emitted, even though iterations won't be. This was a problem before, so the test was explicitly added to test for it when we fixed it.

Not a fan of this bandaid, but maybe it resolves the AppVeyor-exclusive failures. See #1203 (comment)

mstoykov

LGTM;
A slight nit looking at the code

dest := new(...)
*dest = *source

can be simplified as

dest := *source

and using returning &dest
Although I am not very certain if it's not worse in the readability department ... so I don't know if it's good idea to change it given it's a line ;)

imiric · 2019-11-01T12:43:15Z

@mstoykov Also dest := *new(...), right? Not sure, I don't feel strongly either way, though agree that not seeing new() there might trip up Go newcomers such as myself.

I'll leave it as is unless someone insists. :)

na-- · 2019-11-01T14:21:30Z

stats/cloud/collector.go

 			// Check if aggregation is enabled,
 			if c.config.AggregationPeriod.Duration > 0 {
 				newHTTPTrails = append(newHTTPTrails, sc)
 			} else {
 				newSamples = append(newSamples, NewSampleFromTrail(sc))
 			}
 		case *netext.NetTrail:
+			sc = replaceTagsNet(sc)


Wait, wat? 😄 We don't need to replace any tags for iter_li_all/netext.NetTrail. The only tag we need to replace is url, and url only applies to HTTP URLs

:D Right, I wondered about that, but considered we're also tagging network samples with the same tags as HTTP samples to associate them with the same HTTP context.

So is the test added in TestCloudCollector (https://github.com/loadimpact/k6/pull/1203/files#diff-11a7e92298be39c5269425095fb75dc9R283-R326) a fabricated scenario? Meaning NetTrail would never be tagged with url to begin with, right? Because that test was the reason for adding this replacement for NetTrail as well, but it seems unnecessary, or at least should use more realistic tags.

Let me know, but either way, I'll remove this replacement.

Looking at the test now, it should probably be in its own separate test, not a part of the already huge TestCloudCollector()

Meaning NetTrail would never be tagged with url to begin with, right?

Yes, at least not by k6. If users are masochists, they might run k6 with global per-test-run manually set tags like k6 run --tag name=blah --tag url=test script.js, but we don't care about that. As a system tag, k6 only appends url to HTTP requests and websocket connections and name only for HTTP requests.

So yes, please remove the replacement and refactor the test.

OK. Agreed on the separate test. I was lazy and wanted to reuse the existing setup boilerplate in TestCloudCollector, and since it has a nice generic name and already tests HTTP samples, I figured it was a good place for it. :)

But sure, I'll see if I can add a smaller one for this metric change only.

Done, see f032c5f.

It might be a good idea to add tests for Executor.Run (to ensure we're not sending the metric anymore) and for Dialer.GetTrail, but I didn't think it was strictly required, and being lazy again... But let me know if there should be more tests.

na--

See the inline comment - not sure where the tag replacement for netext.NetTrail came from, but it shouldn't happen. The only issue we have is with different url, same name HTTP metrics. The iteration metric doesn't even have an url tag or a name tag.

Resolves: #1203 (comment) Because the iterations metric is now emitted before the minIterationDuration sleep is done--possibly what should've been the correct behavior--it changes these test expectations. And since the previous test wasn't checking for more than one minIterationDuration loop, it makes sense to tweak the settings a bit to gather more data.

Resolves: #1203 (comment)

Resolves: #1203 (comment) Because the iterations metric is now emitted before the minIterationDuration sleep is done--possibly what should've been the correct behavior--it changes these test expectations. And since the previous test wasn't checking for more than one minIterationDuration loop, it makes sense to tweak the settings a bit to gather more data.

Resolves: #1203 (comment)

na--

Besides the minor nit in the test LGTM

stats/cloud/collector_test.go

Closes: #1189

Resolves: #1203 (comment) Because the iterations metric is now emitted before the minIterationDuration sleep is done--possibly what should've been the correct behavior--it changes these test expectations. And since the previous test wasn't checking for more than one minIterationDuration loop, it makes sense to tweak the settings a bit to gather more data.

Resolves: #1203 (comment)

imiric requested review from mstoykov, na-- and cuonglm October 14, 2019 13:50

imiric commented Oct 14, 2019

View reviewed changes

core/engine_test.go Outdated Show resolved Hide resolved

imiric commented Oct 14, 2019

View reviewed changes

js/runner.go Show resolved Hide resolved

imiric commented Oct 14, 2019

View reviewed changes

stats/cloud/collector.go Show resolved Hide resolved

mstoykov requested changes Oct 15, 2019

View reviewed changes

core/engine_test.go Outdated Show resolved Hide resolved

imiric force-pushed the feat/1189-emit-iterations-as-nettrail branch from 904c072 to 6df3a45 Compare October 16, 2019 10:05

na-- previously approved these changes Oct 23, 2019

View reviewed changes

js/runner.go Show resolved Hide resolved

imiric dismissed na--’s stale review via d321c19 October 29, 2019 15:24

imiric force-pushed the feat/1189-emit-iterations-as-nettrail branch from 6df3a45 to d321c19 Compare October 29, 2019 15:24

imiric pushed a commit that referenced this pull request Oct 29, 2019

docs(netext): add TODO comment to refactor netext.Dialer

eb8aa66

Resolves: #1203 (comment)

imiric changed the title ~~WIP: emit iterations metric as part of netext.NetTrail~~ Emit iterations metric as part of netext.NetTrail Oct 29, 2019

imiric marked this pull request as ready for review October 29, 2019 16:01

imiric requested review from mstoykov and na-- October 29, 2019 16:01

na-- reviewed Oct 29, 2019

View reviewed changes

core/engine_test.go Outdated Show resolved Hide resolved

cuonglm previously approved these changes Oct 30, 2019

View reviewed changes

mstoykov previously approved these changes Oct 30, 2019

View reviewed changes

na-- requested changes Oct 30, 2019

View reviewed changes

imiric pushed a commit that referenced this pull request Oct 30, 2019

test(core): add metrics emission test without minIterationDuration

41af367

Resolves: #1203 (comment)

imiric dismissed stale reviews from mstoykov and cuonglm via 41af367 October 30, 2019 12:25

imiric requested a review from na-- October 30, 2019 13:00

na-- requested changes Oct 30, 2019

View reviewed changes

na-- previously approved these changes Oct 30, 2019

View reviewed changes

imiric force-pushed the feat/1189-emit-iterations-as-nettrail branch from b127010 to 484cafd Compare November 1, 2019 11:00

imiric pushed a commit that referenced this pull request Nov 1, 2019

test(core): disable parallel TestMetricsEmission on Windows

b6650a5

Not a fan of this bandaid, but maybe it resolves the AppVeyor-exclusive failures. See #1203 (comment)

mstoykov previously approved these changes Nov 1, 2019

View reviewed changes

na-- reviewed Nov 1, 2019

View reviewed changes

na-- requested changes Nov 1, 2019

View reviewed changes

imiric pushed a commit that referenced this pull request Nov 1, 2019

test(core): add metrics emission test without minIterationDuration

03f614d

Resolves: #1203 (comment)

imiric pushed a commit that referenced this pull request Nov 1, 2019

test(core): disable parallel TestMetricsEmission on Windows

43d818c

Resolves: #1203 (comment)

imiric dismissed mstoykov’s stale review via 43d818c November 1, 2019 15:19

imiric force-pushed the feat/1189-emit-iterations-as-nettrail branch from b6650a5 to 43d818c Compare November 1, 2019 15:19

imiric pushed a commit that referenced this pull request Nov 1, 2019

test(core): add metrics emission test without minIterationDuration

c054ad9

Resolves: #1203 (comment)

imiric pushed a commit that referenced this pull request Nov 1, 2019

test(core): disable parallel TestMetricsEmission on Windows

7c1a49e

Resolves: #1203 (comment)

imiric force-pushed the feat/1189-emit-iterations-as-nettrail branch from 43d818c to 7c1a49e Compare November 1, 2019 17:08

na-- previously approved these changes Nov 1, 2019

View reviewed changes

stats/cloud/collector_test.go Outdated Show resolved Hide resolved

Ivan Mirić added 5 commits November 4, 2019 09:59

feat(cloud): emit iterations metric as part of netext.NetTrail

5de1b50

Closes: #1189

wip(js): emit iterations metric only for default function

34a7278

test(core): add metrics emission test without minIterationDuration

5fae26c

Resolves: #1203 (comment)

test(core): disable parallel TestMetricsEmission on Windows

9cd26e4

Resolves: #1203 (comment)

imiric dismissed na--’s stale review via 9cd26e4 November 4, 2019 09:01

imiric force-pushed the feat/1189-emit-iterations-as-nettrail branch from 7c1a49e to 9cd26e4 Compare November 4, 2019 09:01

mstoykov approved these changes Nov 4, 2019

View reviewed changes

cuonglm approved these changes Nov 4, 2019

View reviewed changes

imiric merged commit 0b94846 into master Nov 4, 2019

imiric deleted the feat/1189-emit-iterations-as-nettrail branch November 4, 2019 15:11

mstoykov added this to the v0.26.0 milestone Nov 7, 2019

na-- mentioned this pull request Nov 19, 2019

Emit the end-of-iteration metrics outside of the networking and JS code #1250

Open

mstoykov mentioned this pull request May 26, 2023

setup() function creates an iteration metric #1605

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Emit iterations metric as part of netext.NetTrail #1203

Emit iterations metric as part of netext.NetTrail #1203

imiric commented Oct 14, 2019 •

edited

Loading

na-- left a comment

codecov-io commented Oct 29, 2019 •

edited

Loading

imiric commented Oct 29, 2019

mstoykov left a comment

na-- left a comment

na-- left a comment

mstoykov left a comment

imiric commented Nov 1, 2019

na-- Nov 1, 2019 •

edited

Loading

imiric Nov 1, 2019

na-- Nov 1, 2019

imiric Nov 1, 2019

imiric Nov 1, 2019

na-- left a comment

na-- left a comment

Emit iterations metric as part of netext.NetTrail #1203

Emit iterations metric as part of netext.NetTrail #1203

Conversation

imiric commented Oct 14, 2019 • edited Loading

na-- left a comment

Choose a reason for hiding this comment

codecov-io commented Oct 29, 2019 • edited Loading

Codecov Report

imiric commented Oct 29, 2019

mstoykov left a comment

Choose a reason for hiding this comment

na-- left a comment

Choose a reason for hiding this comment

na-- left a comment

Choose a reason for hiding this comment

mstoykov left a comment

Choose a reason for hiding this comment

imiric commented Nov 1, 2019

na-- Nov 1, 2019 • edited Loading

Choose a reason for hiding this comment

imiric Nov 1, 2019

Choose a reason for hiding this comment

na-- Nov 1, 2019

Choose a reason for hiding this comment

imiric Nov 1, 2019

Choose a reason for hiding this comment

imiric Nov 1, 2019

Choose a reason for hiding this comment

na-- left a comment

Choose a reason for hiding this comment

na-- left a comment

Choose a reason for hiding this comment

imiric commented Oct 14, 2019 •

edited

Loading

codecov-io commented Oct 29, 2019 •

edited

Loading

na-- Nov 1, 2019 •

edited

Loading