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

Emit iterations metric as part of netext.NetTrail #1203

Merged
merged 5 commits into from
Nov 4, 2019

Conversation

imiric
Copy link
Contributor

@imiric imiric commented Oct 14, 2019

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 Outdated Show resolved Hide resolved
js/runner.go Show resolved Hide resolved
core/engine_test.go Outdated Show resolved Hide resolved
imiric pushed a commit that referenced this pull request Oct 16, 2019
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.
@imiric imiric force-pushed the feat/1189-emit-iterations-as-nettrail branch from 904c072 to 6df3a45 Compare October 16, 2019 10:05
na--
na-- previously approved these changes Oct 23, 2019
Copy link
Member

@na-- na-- left a comment

Choose a reason for hiding this comment

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

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 Show resolved Hide resolved
imiric pushed a commit that referenced this pull request Oct 29, 2019
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.
@imiric imiric force-pushed the feat/1189-emit-iterations-as-nettrail branch from 6df3a45 to d321c19 Compare October 29, 2019 15:24
@codecov-io
Copy link

codecov-io commented Oct 29, 2019

Codecov Report

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

Impacted file tree graph

@@            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.

imiric pushed a commit that referenced this pull request Oct 29, 2019
@imiric 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 imiric marked this pull request as ready for review October 29, 2019 16:01
@imiric imiric requested review from mstoykov and na-- October 29, 2019 16:01
@imiric
Copy link
Contributor Author

imiric commented Oct 29, 2019

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 Outdated Show resolved Hide resolved
cuonglm
cuonglm previously approved these changes Oct 30, 2019
mstoykov
mstoykov previously approved these changes Oct 30, 2019
Copy link
Contributor

@mstoykov mstoykov left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@na-- na-- left a comment

Choose a reason for hiding this comment

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

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.

@imiric imiric dismissed stale reviews from mstoykov and cuonglm via 41af367 October 30, 2019 12:25
@imiric imiric requested a review from na-- October 30, 2019 13:00
Copy link
Member

@na-- na-- left a comment

Choose a reason for hiding this comment

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

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.

na--
na-- previously approved these changes Oct 30, 2019
@imiric 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
Not a fan of this bandaid, but maybe it resolves the AppVeyor-exclusive
failures.

See #1203 (comment)
mstoykov
mstoykov previously approved these changes Nov 1, 2019
Copy link
Contributor

@mstoykov mstoykov left a comment

Choose a reason for hiding this comment

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

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
Copy link
Contributor Author

imiric commented Nov 1, 2019

@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. :)

// 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)
Copy link
Member

@na-- na-- Nov 1, 2019

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

: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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

@na-- na-- left a comment

Choose a reason for hiding this comment

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

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.

imiric pushed a commit that referenced this pull request Nov 1, 2019
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.
imiric pushed a commit that referenced this pull request Nov 1, 2019
@imiric 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
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.
imiric pushed a commit that referenced this pull request Nov 1, 2019
@imiric imiric force-pushed the feat/1189-emit-iterations-as-nettrail branch from 43d818c to 7c1a49e Compare November 1, 2019 17:08
na--
na-- previously approved these changes Nov 1, 2019
Copy link
Member

@na-- na-- left a comment

Choose a reason for hiding this comment

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

Besides the minor nit in the test LGTM

stats/cloud/collector_test.go Outdated Show resolved Hide resolved
Ivan Mirić added 5 commits November 4, 2019 09:59
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.
@imiric imiric merged commit 0b94846 into master Nov 4, 2019
@imiric imiric deleted the feat/1189-emit-iterations-as-nettrail branch November 4, 2019 15:11
@mstoykov mstoykov added this to the v0.26.0 milestone Nov 7, 2019
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.

Emit the iterations metric as part of netext.NetTrail
5 participants