Adding run tags option, apply default tags to all samples per run #553

antekresic · 2018-03-21T14:50:51Z

Closes #461

I changed the option name to --run-tags to keep consistent with the new --system-tags option. --tags looked to general.

… that run

codecov-io · 2018-03-21T14:57:01Z

Codecov Report

Merging #553 into master will increase coverage by 0.57%.
The diff coverage is 69.69%.

@@            Coverage Diff             @@
##           master     #553      +/-   ##
==========================================
+ Coverage    62.3%   62.87%   +0.57%     
==========================================
  Files          96       96              
  Lines        7191     7221      +30     
==========================================
+ Hits         4480     4540      +60     
+ Misses       2445     2429      -16     
+ Partials      266      252      -14

Impacted Files	Coverage Δ
stats/dummy/collector.go	`66.66% <ø> (-3.34%)`	⬇️
lib/options.go	`98.3% <100%> (+34.51%)`	⬆️
core/engine.go	`89.39% <100%> (+0.44%)`	⬆️
cmd/options.go	`38.88% <54.54%> (+5.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 6db2ce5...617cd71. Read the comment docs.

na--

Thanks! Looks mostly good, I think that only some fixes in the tests are necessary and to establish the tag precedence.

@robingustafsson, @luizbafilho: please take a look as well when you have some time

na-- · 2018-03-22T11:14:32Z

core/engine.go

+		}
+
+		for k, v := range e.Options.RunTags {
+			samples[i].Tags[k] = v


Hm, I'm not sure what the precedence for the tags should be. If the user specifies a tag with --run-tags mytag=myval1 and then has export let options = { runTags : {mytag: "myval2"} } and http.get("https://httpbin.org/", { tags: { mytag: "myval3" } }); in the code, what tag value should be emitted? I think the order should be myval3, if not present myval1, finally (with the least priority) myval2, but I'm not completely sure that's the correct approach.
@robingustafsson: thoughts?

Hm, yeah I think the cascading order should be Request level -> CLI -> Env vars -> Options -> Config file (where -> means it overrides whatever is on the right). So more or less the same as we do already, besides the request-level case: https://github.com/loadimpact/k6/blob/master/cmd/run.go#L140

The only reason I can think of why the request level should not override all other possible option inputs is that you won't be able to override it from outside of the script (using CLI or Env vars). However, in reality that might not be an issue as one can argue that if you have specified a tag on the request level it's probably for a very good reason.

Yeah, agree, and you can "override" tags you've specified at the request level from the CLI if you just use environment variables in the code tags 🙂

So here the for loop should check if Tags[k] is present before saving the value, I think that should be enough.

na-- · 2018-03-22T11:27:10Z

core/engine_test.go

+			}()
+
+			for !c.IsRunning() {
+				time.Sleep(time.Millisecond)


See the notes about the IsRunning() method below, this should be refactored. Or possibly removed, I'm not sure if it's even needed and if we actually need to have a context, cancel it, Run() the collector and check when it finishes... This is a test and we know it's the Dummy collector anyway, so can't we skip all of that?

na-- · 2018-03-22T11:28:14Z

core/engine_test.go

+
+			//wait for the collector to start running
+			for !c.IsRunning() {
+				time.Sleep(time.Millisecond)


Same thing applies here as in the comment above

na-- · 2018-03-22T11:41:12Z

stats/dummy/collector.go

+
+// IsRunning returns the current status of the collector
+func (c *Collector) IsRunning() bool {
+	return c.running


This will cause a race condition since it doesn't lock (see Run()), but that's beside the point. Generally in Go it's not a very good idea to have methods like this, because the only way they can be used is with continuous polling, like you've done with the time.Sleep(time.Millisecond), and it's also easier to introduce race conditions. It's much, much better to use channels or something like sync.WaitGroup (or even sync.Cond for particularly tricky cases) for this type of synchronization.

na-- · 2018-03-22T11:43:36Z

lib/options_test.go

+		opts := Options{}.Apply(Options{SummaryTrendStats: stats})
+		assert.Equal(t, stats, opts.SummaryTrendStats)
+	})
+	t.Run("RunTags", func(t *testing.T) {


All of the other changes besides this one are out of the scope for this pull request. I don't mind very much, but ideally they should have been in a separate PR, which I would have reviewed and merged in a few minutes. Now they have to wait for the other more complicated things here to be fixed, and they clutter the code review and confuse the commit history a bit.

I agree, separate PR would have been better for other tests. I can remove them from this PR if necessary.

As I said, I don't mind all that much and they're already part of the history anyway. At this point I think it's better to leave them as they are, since the alternatives are to either force push the branch or create a new pull request from a new branch (and loose the context and comments already here)

na-- · 2018-03-22T11:46:46Z

stats/dummy/collector_test.go

@@ -52,7 +52,9 @@ func TestCollectorCollect(t *testing.T) {
 		ctx, cancel := context.WithCancel(context.Background())
 		defer cancel()
 		go func() { c.Run(ctx) }()
-		time.Sleep(1 * time.Millisecond)


Hmm, it's not good that there was a sleep here, I think that I will probably have to refactor the Dummy collector a bit... The whole running variable should probably go, it shouldn't be needed since we have the context...

I agree about the Dummy collector, I have mentioned the issue here: #469 (comment)

Changing it would avoid all the "subpar" changes I made to the tests in order to get them to work.

Ah, I missed this comment before posting again in the general PR thread. OK, I'm convinced we should refactor the Dummy collector. Not sure if my suggestion with using a channel instead of a Sample slice is the best way to go about it, but I don't have any other ideas.

Maybe we can just remove the running check with panic() for this pull request and create a new issue and PR for the actual Dummy collector refactoring... @robingustafsson?

One idea is to change the Dummy collector implementation by adding a buffered channel to signal the running start and use that instead of the polling solution in the tests. That should get rid of the nastiness in there and we can refactor the rest in a separate PR.

I'm not sure we actually need to signal anything, the dummy collector is used just for tests, so we basically just have to have an easy way to read the collected samples. I think that we could just replace the current []stats.Sample with a Samples channel (buffered or not, both could be useful and the buffer size will be supplied when the dummy collector is created anyway) for stuffing the results in the collector and reading them outside with range. When the context is closed, close the Samples channel an that's that.

But that should probably be a part of another PR for #469 or for a new issue. Do you think that simply removing this check temporarily (before that other PR is created) will be enough so we don't need all of that nasty polling and sleep?

I guess so. And I would have to change the stats/dummy/collector_test.go test file. That would fix the issue on #469 as well.

na-- · 2018-03-22T12:18:39Z

Looking at this a bit more closely, I'm not sure if you can easily get rid of all of the polling and sleep with how the dummy collector currently works. I think that maybe we should refactor the dummy collector to actually use an (optionally buffered) stats.Sample channel instead of the current slice Samples []stats.Sample, I think that it would be much easier to test things reliably then.

It's probably also worth it to investigate how the collectors are used in general and see if some refactoring of the Collector interface won't greatly simplify things, but that's hopefully not needed for this PR and can be a separate issue for the future.

na--

LGTM now, I'll add a note to #469 about the fixes of the Dummy collector we discussed here

robingustafsson · 2018-03-26T09:47:42Z

The code LGTM, well done @antekresic!

The only issue I have is with the naming and CLI UX :)

I think --run-tags should be --tags. I can't think of a need to reserve --tags (or --tag, see below) for some other purpose, so I'd vote for the shorter --tags name.

Another thing I started thinking about is consistency in the CLI. This option is implemented in plural form (--run-tags "NAME=VAL, NAME2=VAL2" vs --tag NAME=VAL --tag NAME2=VAL2). The other key/value we have, env vars, is implemented in singular form (-e NAME=VAL). Then there's a mix for other multi-value options:

Singular:

stages (-s Duration1:Target1 -s Duration2:Target2)

Plural:

IP blacklisting (--blacklist-ip "CIDR1,CIDR2")
Summary trend stats (--summary-trend-stats "stat1,stat2")

What are your thoughts on this?

antekresic · 2018-03-26T10:33:42Z

Always the hardest part, naming things :)

Looking at all the options of the run command, I do see somewhat of a distinction emerging among multi-value options. Some of them are adding new values and others are redefining some already predefined values. That might be a subtle difference that we could express by using different forms.

For instance, --blacklist-ip is adding a new blacklist IP range (none are defined by default) so singular makes sense. --summary-trend-stats and --system-tags, on the other hand, are redefining default values so for me it makes sense to keep them in plural. Using this rule, we should probably use --tag for this feature and it already supports multiple option values in the same command so it would work nicely.

At the end of the day, for me, it all comes down to consistency. Pick a rule and stick to it :)

Adding run tags support, tags that will be applied to all samples for…

e60a459

… that run

na-- requested changes Mar 22, 2018

View reviewed changes

antekresic added 2 commits March 22, 2018 13:48

Adding check to prevent overwritting existing tags on samples

3c7ec4f

Removing running check from dummy collector to improve testability

cd23bc4

na-- approved these changes Mar 22, 2018

View reviewed changes

na-- mentioned this pull request Mar 22, 2018

Fix flakiness of engine tests #469

Closed

na-- mentioned this pull request Mar 26, 2018

Simplify the dummy collector #561

Merged

Changed option name to tag

617cd71

robingustafsson approved these changes Mar 26, 2018

View reviewed changes

robingustafsson merged commit 978004d into grafana:master Mar 26, 2018

antekresic deleted the feature/addRunTags branch April 4, 2018 10:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding run tags option, apply default tags to all samples per run #553

Adding run tags option, apply default tags to all samples per run #553

antekresic commented Mar 21, 2018

codecov-io commented Mar 21, 2018 •

edited

Loading

na-- left a comment

na-- Mar 22, 2018 •

edited

Loading

robingustafsson Mar 22, 2018

na-- Mar 22, 2018

na-- Mar 22, 2018

na-- Mar 22, 2018

na-- Mar 22, 2018

na-- Mar 22, 2018

antekresic Mar 22, 2018

na-- Mar 22, 2018

na-- Mar 22, 2018

antekresic Mar 22, 2018

na-- Mar 22, 2018 •

edited

Loading

antekresic Mar 22, 2018

na-- Mar 22, 2018

antekresic Mar 22, 2018

na-- commented Mar 22, 2018

na-- left a comment

robingustafsson commented Mar 26, 2018

antekresic commented Mar 26, 2018

Adding run tags option, apply default tags to all samples per run #553

Adding run tags option, apply default tags to all samples per run #553

Conversation

antekresic commented Mar 21, 2018

codecov-io commented Mar 21, 2018 • edited Loading

Codecov Report

na-- left a comment

Choose a reason for hiding this comment

na-- Mar 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

na-- Mar 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

na-- commented Mar 22, 2018

na-- left a comment

Choose a reason for hiding this comment

robingustafsson commented Mar 26, 2018

antekresic commented Mar 26, 2018

codecov-io commented Mar 21, 2018 •

edited

Loading

na-- Mar 22, 2018 •

edited

Loading

na-- Mar 22, 2018 •

edited

Loading