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

summarize could show min/max times and other useful stats. #67

Open
eddyb opened this issue Oct 16, 2019 · 7 comments
Open

summarize could show min/max times and other useful stats. #67

eddyb opened this issue Oct 16, 2019 · 7 comments
Labels
enhancement New feature or request good first issue Good for newcomers T-summarize Related to the summarize tool

Comments

@eddyb
Copy link
Member

eddyb commented Oct 16, 2019

We're investigating a case where a query is taking 30-40ms on average, but by using crox we can see instances of that query being over 200ms sometimes, with the maximum around 330ms.

Would be nice to get this information from summarize itself.

@theotherphil
Copy link

I'd like to have a go at this, if it's not particularly time sensitive.

@michaelwoerister
Copy link
Member

A related question to @Mark-Simulacrum: Will perf.rlo crash if new columns are added or will it just ignore them?

@michaelwoerister
Copy link
Member

@theotherphil Feel free to give it a try!

@Mark-Simulacrum
Copy link
Member

I'm not sure what you mean by columns. I expect no - perf.rlo parses JSON summaries with serde and iirc doesn't prohibit unknown fields and such, though it may come down to the specifics of the data format. I don't expect problems though.

@michaelwoerister
Copy link
Member

michaelwoerister commented Jan 9, 2020

With "column" I mean something that could end up as a new column in the detailed view table, so in practice a new field in summarize's QueryData struct.

I gave it a try earlier and, as you say, serde_json seems to handle unknown fields just fine: https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=4c8b1574ce2e46c4e4687d88e7938c98

@theotherphil
Copy link

I've just had a look at this.

I want to start by adding min and max, to get the wiring sorted before I worry about the overhead of tracking the state required to compute percentiles.

I'm a bit stuck at the very first hurdle: I'm not sure how to recognise a complete logical event in the event data. Specifically, I'm not sure what relationship is guaranteed between QUERY_EVENT_KIND, INCREMENTAL_LOAD_RESULT_EVENT_KIND and QUERY_BLOCKED_EVENT_KIND events in rustc's self-profiler output. #104 adds the time from incremental cache loading and blocked queries to the self time for each event label. Should I attempt to group consecutive events with different kinds but matching labels, or just ignore the latter two event kinds?

@michaelwoerister
Copy link
Member

@theotherphil Each event is self-contained. For min/max you can just compare Event::timestamps. I would not make a distinction between the different event kinds.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers T-summarize Related to the summarize tool
Projects
None yet
Development

No branches or pull requests

5 participants