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

Add "scenario" as default system tag #1518

Merged
merged 2 commits into from
Jun 30, 2020
Merged

Conversation

imiric
Copy link
Contributor

@imiric imiric commented Jun 29, 2020

This tags all metrics emitted in the main "exec" function with a "scenario" tag set to the scenario name or "default" if no custom scenarios were defined. It can be disabled with the --system-tags option.

There's no issue for this specific feature, but see #796 and #1300.

Ivan Mirić added 2 commits June 29, 2020 10:54
This tags all metrics emitted in the main "exec" function with a "scenario" tag
set to the scenario name or "default" if no custom scenarios were defined.
It can be disabled with the `--system-tags` option.

There's no issue for this specific feature, but see #796 and #1300.
@imiric imiric requested review from mstoykov and na-- June 29, 2020 11:38
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! We really should refactor runFn... eventually... 😅

@codecov-commenter
Copy link

codecov-commenter commented Jun 29, 2020

Codecov Report

Merging #1518 into new-schedulers will increase coverage by 0.05%.
The diff coverage is 100.00%.

Impacted file tree graph

@@                Coverage Diff                 @@
##           new-schedulers    #1518      +/-   ##
==================================================
+ Coverage           76.64%   76.69%   +0.05%     
==================================================
  Files                 163      163              
  Lines               13044    13047       +3     
==================================================
+ Hits                 9998    10007       +9     
+ Misses               2532     2527       -5     
+ Partials              514      513       -1     
Impacted Files Coverage Δ
stats/system_tag.go 81.13% <ø> (ø)
stats/system_tag_set_gen.go 61.53% <ø> (ø)
js/runner.go 82.62% <100.00%> (+0.12%) ⬆️
lib/executor/helpers.go 96.73% <100.00%> (+2.23%) ⬆️
lib/executor/vu_handle.go 93.69% <0.00%> (-1.81%) ⬇️
stats/cloud/collector.go 78.70% <0.00%> (+0.61%) ⬆️
lib/executors.go 94.11% <0.00%> (+1.96%) ⬆️
lib/testutils/minirunner/minirunner.go 81.48% <0.00%> (+3.70%) ⬆️

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 a9903e0...62bed13. Read the comment docs.

Comment on lines +507 to +509
if scenario != "" && opts.SystemTags.Has(stats.TagScenario) {
tags["scenario"] = scenario
}
Copy link
Contributor

Choose a reason for hiding this comment

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

So we don't want to let people overwrite scenario as a tag ?

Copy link
Member

Choose a reason for hiding this comment

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

hmm 😕 what's stopping them from overwriting it in the per-request tags?

Copy link
Contributor

Choose a reason for hiding this comment

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

we merge them above in tags and here we set tags[scenario] regardless :). Same btw goes for iter, vu and group. To be honest, I am not against us overwriting them, but maybe at least have some log about it.

Although this will probably need to be as part of #1478 as here it will be too noisy

Copy link
Member

Choose a reason for hiding this comment

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

hmm the tags we merge with above are the VU activation tags, I think... so yes, I don't think you should be able to overwrite the k6 system tags with the per-scenario tags

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I also considered it a deliberate design choice, especially since there's a workaround: they can create a custom tag with the same name and use --system-tags to disable the built-in ones. So I'm OK with this approach too.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I am also okay with us overwriting it. I would just prefer if we log it somewhere when it happens.

With #1478 I can do it once per initialization ... which is probably way too noisy still ... :(

Copy link
Contributor

Choose a reason for hiding this comment

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

but you can (and should) just merge this for now :D

@imiric imiric merged commit 0411270 into new-schedulers Jun 30, 2020
@imiric imiric deleted the feat/default-scenario-tags branch June 30, 2020 07:50
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.

4 participants