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

Proposal to modify the group tag syntax. #948

Closed
ppcano opened this issue Mar 12, 2019 · 3 comments
Closed

Proposal to modify the group tag syntax. #948

ppcano opened this issue Mar 12, 2019 · 3 comments
Labels
enhancement evaluation needed proposal needs to be validated or tested before fully implementing it in k6 ux

Comments

@ppcano
Copy link
Contributor

ppcano commented Mar 12, 2019

The current syntax of the system tag group is not fully documented and its syntax may not be intuitive.

Given the following script:

import http from "k6/http";
import { check, group } from "k6";


export default function() {
 group("mainGroup", function() {
   http.get("https://loadimpact.com");

   group("nestedGroup", function() {
     http.get("https://test.loadimpact.com");
   });
 });
};

It will generate tags like:

{"type":"Point","tags":{"group":"::mainGroup"}},"metric":"http_reqs"...}
{"type":"Point","tags":{"group":"::mainGroup::nestedGroup"}},"metric":"http_reqs"...}

{"type":"Point", "tags":{"group":"::mainGroup"}},"metric":"group_duration"...}
{"type":"Point", "tags":{"group":"::mainGroup::nestedGroup"}},"metric":"group_duration"...}

The current syntax is that group tags are always prepended with ::. A threshold on a non-nested group must be configured:

export let options = {
  thresholds: {
    "group_duration{group:::mainGroup}": ["p(95)<1000"],
  }
}

The syntax of 👆 is not intuitive. I think :: is a good symbol for referencing a nested group, but the first group may not be perceived as a nested group.

My proposal is to remove :: from non-nested groups, then users could define thresholds as:

export let options = {
  thresholds: {
    "group_duration{group:mainGroup}": ["p(95)<1000"],
    "group_duration{group:mainGroup::nestedGroup}": ["p(95)<1000"],
  }
}
@ppcano ppcano changed the title Modify the group tag syntax. Proposal to modify the group tag syntax. Mar 12, 2019
imiric pushed a commit that referenced this issue Aug 23, 2019
@na-- na-- added the evaluation needed proposal needs to be validated or tested before fully implementing it in k6 label Jan 26, 2021
@na--
Copy link
Member

na-- commented Jan 26, 2021

Hmm while I don't like the :: separator, I don't think we can change it now, it would be a major breaking change. Besides, we can fix the issue from the other side, by changing the threshold syntax so it's not depending on the colon for equality, i.e. get rid of key:value expressions for submetrics.

We want to do that for other reasons as well, the current submetric definition is quite inflexible. See #1443 and #1313 for more details. And whatever that syntax ends up being, if we have something like group === "::mainGroup::nestedGroup", it looks much better than the current ::: mess. So, I'll make a note in #1443 about this, and I think we should close this issue and #1119. @imiric @mstoykov, thoughts?

@mstoykov
Copy link
Collaborator

Yeah ... I don't think breaking this is good idea. So I guess at least making less strange in the thresholds is better

@na--
Copy link
Member

na-- commented Jan 26, 2021

Ok, closing this and the PR then.

@na-- na-- closed this as completed Jan 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement evaluation needed proposal needs to be validated or tested before fully implementing it in k6 ux
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants