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

Scenario's stage number #2199

Closed
wants to merge 2 commits into from
Closed

Scenario's stage number #2199

wants to merge 2 commits into from

Conversation

codebien
Copy link
Contributor

@codebien codebien commented Oct 25, 2021

Adding scenario's stage id information.
Export the property exec.scenario.stage.id that returns a zero-based value tracking for the current running stage.

An example using it for tracking the stage as a tag:

import http from 'k6/http';
import exec from 'k6/execution';

export const options = {
  stages: [
    { duration: '1m', target: 10 },
    { duration: '1m', target: 20 },
    { duration: '1m', target: 0 },
  ],
};

export default function () {
  exec.vu.tags["stage"] = 'stage#${exec.scenario.stage.id}'
  http.get('http://test.k6.io/');
}

@codebien codebien self-assigned this Oct 25, 2021
@codebien codebien changed the title Scenario stage number Scenario's stage number Oct 25, 2021
@codebien codebien force-pushed the vu-tags branch 2 times, most recently from 3790391 to d3e4d1f Compare October 26, 2021 16:01
@na-- na-- added this to the v0.35.0 milestone Oct 27, 2021
The stage object shares information about the current scenario's stage.
Added a StagesDurations for tracking Scenario's Stages. Populate it from
Ramping VUs and Ramping arrival rate executors.
@codebien codebien changed the base branch from vu-tags to master October 28, 2021 08:14
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.

I don't think trying to calculate the exact stage you are in based on time every time exec.scenario.stage.number is accessed is a good idea at all.

As I've mentioned I doubt most users will care about that, they probably just want in which stage it started. On the other hand (as I mentioned inline) this will IMO be really confusing when it starts changing values and will lead to really hard to understand bugs.

This also is somewhat less logical for an iteration based executor such as ramping-arrival-rate as that iteration started in the "ramping-up" part , the fact that the executor is now in the "ramping-down" (after 10 minutes of "steady" stage) is probably interesting, but I would argue that this iteration is still from "ramping-up" stage ;) . With ramping-vus I would argue the opposite is more coherent. Maybe we should have exec.iteration.stage and exec.scenario.stage 🤷

I am also not really certain about the whole name stuff ... and at this point I feel like maybe we should scrap all this and just have per stage tags and env variables as we have per scenario and call it a day 🤷.

Comment on lines +577 to +579
stages := make([]lib.ScenarioStage, 0, len(vlv.config.Stages))
for i, s := range vlv.config.Stages {
stages = append(stages, lib.ScenarioStage{
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: you can just make with the appropriate len(instead of cap) and do stages[i] = ... instead of append

Comment on lines +134 to +137
if len(s.Stages) < 1 {
// TODO: improve this error message
return nil, fmt.Errorf("can't get the current Stage because any Stage has been defined")
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This is going to throw an exception on executors with no stages? I am not certain this is the best idea instead of returning undefined for example, but 🤷 works as well.

Comment on lines +139 to +149
// sum represents the stages passed
sum := int64(0)
elapsed := time.Since(s.StartTime)
for _, stage := range s.Stages {
sum += int64(stage.Duration)
// when elapsed is smaller than sum
// then the current stage has been found
if int64(elapsed) < sum {
return &stage, nil
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't really think that trying to calculate the exact stage on each call is a good idea. I really don't think that will be what most users will care about and if they do there is an easy way to do it (as you do here).

But I would expect it will be really surprising to people accessing exec.scenario.stage.number in two different parts of their iteration and it ... changing.

I do expect that later one will definitely get them to scratch their head quite a bit and try to figure out how did they write their code in such a way that a key they just added with exec.scenario.stage.number is now not there 🤔

@codebien
Copy link
Contributor Author

codebien commented Oct 28, 2021

I am also not really certain about the whole name stuff ... and at this point I feel like maybe we should scrap all this and just have per stage tags and env variables as we have per scenario and call it a day shrug.

@mstoykov The main problem with this it's the ramping-vus executor, because we don't have a defined instant when the stage is changing, so we can't set the tags for it. Am I missing something?

@na--
Copy link
Member

na-- commented Oct 29, 2021

@mstoykov we've had this discussion already, and the consensus (or so I thought) was that if we had this stage number, users will be able to cover every use case reasonably easily and logically:

  • If you want to get which stage the iteration is in, query exec.scenario.stage.number at the start of the iteration and save the result in a variable.
  • If you want to get which stage a particular HTTP request actually occurs in, query the stage.number before/after you make it.
  • If you want extra tags, use exec.vu.tags to add them
  • If you want extra env vars for different stages, you can have a map and use the stageNumber to look them up

Regarding the rest, I think we should hold off on stage.name... IIRC we haven't decided if that would automatically add tags or not yet. And stage.number should probably be renamed to stage.id to be consistent with the rest of the k6/execution properties 🤔

@codebien
Copy link
Contributor Author

Updated the description adding a small example, I will remove the name property.

@mstoykov
Copy link
Contributor

mstoykov commented Nov 1, 2021

@na-- I am very much against exec.scenario.stage.number==exec.scenario.stage.number being able to return false.

I do think this will be way more confusing and add bugs than if exec.scenario.stage.number is just a constant.

All of the above other usecases, which I doubt are all that interesting for most users - I would expect most just want to know "vaguely" whether this iteration started in a given stage, after all as you've eluded that will change and it can change also over the course of a request, and we are not even doing asynchronous code yet. But I digress ... all of the above other usecases can be handled through js code, which is already possible as @codebien has shown. And as I have argued time and time again, adding functionality that can be implemented outside of k6 means that we still have to document it(which we have to do for the js code as well), but now we also can't change it, while we can always release another jslib showing how to use something else but as v0.2.0 ;)

If anything in order to make it easier I am more for exec.scenario.stages being a "copy" of what the stages in the configuration for the given scenario are. This is unlikely to have to change and will be helpful for implementing the above use case as well as anything else.

Also again, can we discuss API changes with examples in the issues at least along implementing them if not before that?

@na-- na-- modified the milestones: v0.35.0, v0.36.0 Nov 2, 2021
@na--
Copy link
Member

na-- commented Nov 2, 2021

I moved the milestone for this to v0.36.0. I disagree with you and think something like this is probably the best solution, but let's discuss it in #2215 after we ship v0.35.0...

@na-- na-- modified the milestones: v0.36.0, v0.37.0 Jan 11, 2022
@codebien codebien removed this from the v0.37.0 milestone Feb 23, 2022
@codebien
Copy link
Contributor Author

This isn't in the current plan, we could reconsider this in the future but at the moment is not going to be merged. For the current plan check: #796 (comment)

@codebien codebien closed this Apr 11, 2022
@na-- na-- deleted the scenario-stageNumber branch April 11, 2022 13:02
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.

3 participants