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.

Sign up for GitHub

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

Jump to bottom

update iter/s precision from 0 to 2 #2282

Merged
merged 1 commit into from
May 18, 2022
Merged

update iter/s precision from 0 to 2 #2282

merged 1 commit into from
May 18, 2022

Conversation

m3hm3t
Copy link
Contributor

@m3hm3t m3hm3t commented Dec 2, 2021

This Pull Request addresses issue 1753.

If you set up a scenario where the executors run less than one iteration per second, the value is floored to zero while in progress.

By this PR, the behavior changes like at the below.

From:
image

To:
image

@CLAassistant
Copy link

CLAassistant commented Dec 2, 2021 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@github-actions github-actions bot requested review from codebien and na-- December 2, 2021 18:48
@m3hm3t m3hm3t mentioned this pull request Dec 2, 2021
Closed
@mstoykov
Copy link
Contributor

mstoykov commented Dec 3, 2021

Hi @m3hm3t thanks for the PR, unfortunately, this was already kind of tried in #1766.

And as I mention there it looks really strange with ramping-arrival-rate and big timeUnit. I don't think we have agreed on my suggestion, but maybe you can experiment with it? @darkaether seems to have not had time (which is completely understandable) do to so, so it's fine if somebody else does :).

@na-- na-- requested review from mstoykov and removed request for na-- December 3, 2021 09:22
@m3hm3t
Copy link
Contributor Author

m3hm3t commented Dec 3, 2021

Thanks for the comment @mstoykov. I will look into it.

codebien
codebien previously requested changes Dec 3, 2021
View reviewed changes
Copy link
Contributor

@codebien codebien left a comment

Choose a reason for hiding this comment

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

Setting the status as request changes

@oleiade oleiade added this to the v0.39.0 milestone May 12, 2022
@mstoykov
Copy link
Contributor

After some internal discussion we have agreed that having this is probably better than nothing. Also some people have pointed out that changing the timeUnit shown will likely be more confusing than anything else.

mstoykov
mstoykov approved these changes May 12, 2022
View reviewed changes
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! thanks for the PR and sorry for the really long delay :(

@mstoykov mstoykov requested a review from codebien May 12, 2022 12:23
@mstoykov mstoykov mentioned this pull request May 12, 2022
Closed
oleiade
oleiade approved these changes May 16, 2022
View reviewed changes
Copy link
Member

@oleiade oleiade left a comment

Choose a reason for hiding this comment

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

👍🏻 🦕

@mstoykov mstoykov merged commit 05635c2 into grafana:master May 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oleiade oleiade oleiade approved these changes

@mstoykov mstoykov mstoykov approved these changes

@codebien codebien Awaiting requested review from codebien

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v0.39.0
Development

Successfully merging this pull request may close these issues.
5 participants
@m3hm3t @CLAassistant @mstoykov @oleiade @codebien