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

Various changes to perf code #3037

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Stevenjin8
Copy link
Contributor

No description provided.

@istio-testing
Copy link
Contributor

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@istio-testing istio-testing added do-not-merge/work-in-progress Block merging of a PR because it isn't ready yet. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Sep 23, 2024
@Stevenjin8 Stevenjin8 changed the title Don't put rustfilt in cargo path Various changes to perf code Sep 24, 2024
@Stevenjin8 Stevenjin8 force-pushed the perf/small-fixes branch 5 times, most recently from 48a81ab to a65a20b Compare September 24, 2024 17:53
@@ -174,15 +179,20 @@ def get_parser():
parser.add_argument(
"--query_str",
help="Specify the qps or conn query_str that will be used to query your y-axis data based on the CSV file."
"For example: conn_query_str=ActualQPS==1000, qps_query_str=NumThreads==16."
"For example: conn_query_str=ActualQPS==1000, qps_query_str=NumThreads==16.",
Copy link
Member

Choose a reason for hiding this comment

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

not sure what that example is but if ActualQPS is from fortio json, it's very unlikely to be a nice exact round number - if you ask for 1000 you're likely to get 999.97 or some such

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think my formatter might have caught this. Having the hard equals isn't great, I agree. But it gets rounded to an int, so if you run it for long enough, the actual qps ends up being the desired qps (most of the time).

else:
if not data[metric_name].head().empty:
if metric_name.startswith('cpu') or metric_name.startswith('mem'):
y_series_data.append(data[metric_name].head(1).values[0])
else:
y_series_data.append(data[metric_name].head(1).values[0] / data["ActualQPS"].head(1).values[0])
y_series_data.append(data[metric_name].head(1).values[0] / 1000)
Copy link
Member

Choose a reason for hiding this comment

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

not sure why it was using data["ActualQPS"] before but maybe comment on the change?
(also fortio data, in the json files, is in seconds not ms (even if the UI shows in ms for human consumption) so not sure what the 1000 is for either?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After some digging I think this is correct (but a comment would help). Fortio reports data in seconds, but the script that pull data from the fortio client converts it to microseconds

p = h["Percentiles"]
for pp in p:
obj["p" + str(pp["Percentile"]).replace(".", "")
] = int(pp["Value"] * 10 ** 6)

so I have to divide by 1000 to get it back to milliseconds

@Stevenjin8 Stevenjin8 requested a review from ldemailly November 1, 2024 23:09
@Stevenjin8 Stevenjin8 marked this pull request as ready for review November 1, 2024 23:10
@Stevenjin8 Stevenjin8 requested a review from a team as a code owner November 1, 2024 23:10
@istio-testing istio-testing removed the do-not-merge/work-in-progress Block merging of a PR because it isn't ready yet. label Nov 1, 2024
@Stevenjin8 Stevenjin8 self-assigned this Nov 1, 2024
@Stevenjin8 Stevenjin8 added the do-not-merge Block automatic merging of a PR. label Nov 1, 2024
@istio-testing
Copy link
Contributor

@Stevenjin8: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
gencheck_tools 2ee663f link true /test gencheck
lint_tools 2ee663f link true /test lint

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do-not-merge Block automatic merging of a PR. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants