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

KIALI-2207 Do not round timeseries when rounded result affect signifi… #773

Merged
merged 1 commit into from Jan 9, 2019

Conversation

@jotak
Copy link
Contributor

commented Jan 8, 2019

…cant decimals

JIRA: https://issues.jboss.org/browse/KIALI-2207

Screenshot before:
Before

Screenshot after:
After

@jotak jotak requested review from jshaughn and israel-hdez Jan 8, 2019
@jotak

This comment has been minimized.

Copy link
Contributor Author

commented Jan 8, 2019

@jshaughn / @israel-hdez I think this is ok with graph side-panel too, but let me know if you think it's not

@jotak jotak force-pushed the jotak:kiali-2207 branch from 61ec249 to ae03024 Jan 8, 2019
@jshaughn

This comment has been minimized.

Copy link
Contributor

commented Jan 8, 2019

@jotak That's interesting PromQL. It doesn't cause the inner query to be performed twice, right? So, no real perf impact?

@jotak

This comment has been minimized.

Copy link
Contributor Author

commented Jan 8, 2019

@jshaughn I'm not sure if Prometheus caches the result internally, I hope so, but at least I guess it wouldn't run the right operand of "OR" if the left operand is true, which should stands for the vast majority of the cases.

@jotak

This comment has been minimized.

Copy link
Contributor Author

commented Jan 8, 2019

@simonpasquier any idea about the performance impact of this?

@simonpasquier

This comment has been minimized.

Copy link
Contributor

commented Jan 8, 2019

So unless I'm mistaken, Prometheus is going to evaluate twice the inner query as it doesn't do deduplicating when parsing the whole query.

In practice if we run round(rate(foo[1m]), 0.1) > 0.1 or rate(foo[1m]), Prometheus should evaluate the first rate(foo[1m]) then round(..., 0.1) > 0.1 drops all samples below the threshold then it will evaluate the second rate(foo[1m])and finally it will return all series from the first query + all series from the second query that aren't already present in the first set.

Obviously it is a performance hit but I think it should be reasonable and hardly noticeable unless you're returning 10s of thousands of series.

Copy link
Contributor

left a comment

@jotak OK, so it seems there will be perf impact to take care of this corner case but the impact is likely not terrible. The only alternative I see would be to perform the query and then check the results for 0.001 and then redo things if necessary. I doubt it's worth that added complexity.

Copy link
Member

left a comment

I'm OK with the change.

However, since this is only covering one very specific case, makes me wonder if it's OK to round (or "process") numbers at server side. For me, it would be better if rounding happens at UI side, where needed. But that's a larger effort. So, this is OK.

@rhqci

This comment has been minimized.

Copy link
Collaborator

commented Jan 9, 2019

Jenkins CI: kiali-core-pr-e2e-test #382

  • ✔️ run-kiali-e2e-tests #[963]
@rhqci

This comment has been minimized.

Copy link
Collaborator

commented Jan 9, 2019

!!! Couldn't read commit file !!!

@jotak

This comment has been minimized.

Copy link
Contributor Author

commented Jan 9, 2019

@jshaughn @israel-hdez so I'm merging it but also opened a ticket here for client-side rounding: https://issues.jboss.org/browse/KIALI-2217
I haven't assigned a team so feel free to take it if you want, or I will do later.

@jotak jotak merged commit 72c40b3 into kiali:master Jan 9, 2019
3 checks passed
3 checks passed
Jenkins-CI Test PASSed.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
security/snyk - tests/e2e/requirements.txt (theute) No manifest changes detected
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.