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

Admin UI Statements page - Reference doc #3300

Merged
merged 6 commits into from
Jun 28, 2018
Merged

Conversation

Amruta-Ranade
Copy link
Contributor

@Amruta-Ranade Amruta-Ranade commented Jun 27, 2018

To-Do (for future sprints):

  • Edit the doc as required (change navigation guidance after the page is made accessible from the navigation bar, edit limitations as they are resolved, add screenshots after page design is finalized).
  • Identify and update related documents (Troubleshooting guidance, production docs, etc.)

@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Contributor

@couchand couchand left a comment

Choose a reason for hiding this comment

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

This is great, thanks for turning it around so quick!


## Limitations

- If you have multiple applications running on the cluster, the **Statements** page shows cumulative parameter values across all applications, while the **Statements Details** page shows the values for the first application only.
Copy link
Contributor

Choose a reason for hiding this comment

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

Unfortunately, it's worse than the first clause says. The Statements page doesn't aggregate in this alpha release, so it shows a row for each of the applications. The problem here is that there isn't a way to tell which application is which. I'm not sure the best way to document that. 😞


## Limitations

- If you have multiple applications running on the cluster, the **Statements** page shows cumulative parameter values across all applications, while the **Statements Details** page shows the values for the first application only.
Copy link
Contributor

Choose a reason for hiding this comment

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

It might be useful to link to documentation about setting the application, but as far as I can tell it's not really documented on it's own, just briefly on the cluster setting itself and then randomly mentioned elsewhere.


### SQL statement fingerprint

Whenever possible, the **Statements** page displays the details of SQL statement fingerprints instead of individual SQL statements.
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like this is a little bit confusing. The way I think about it is that there are some statements where the fingerprint happens to be the same as the statement itself, notably when they contain no constant values. We're still using the fingerprint. But this sentence makes it sound less orderly or something...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@couchand Hmmm....right. So effectively, even if the fingerprint is the statement itself, it is still a fingerprint. I think I can remove the "Whenever possible" part. Right?

`INSERT INTO new_order(product_id, customer_id, transaction_id) VALUES (380, 11, 11098)`
`INSERT INTO new_order(product_id, customer_id, transaction_id) VALUES ($1, 11, 11098)`
`INSERT INTO new_order(product_id, customer_id, transaction_id) VALUES ($1, $2, 11098)`
`INSERT INTO new_order(product_id, customer_id, transaction_id) VALUES ($1, $2, $3)`
Copy link
Contributor

Choose a reason for hiding this comment

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

I've opened cockroachdb/cockroach#27052 to track the idea to use placeholders for fingerprints. Pending the way that gets resolved these examples may move up to the block above!

Time | The cumulative time taken to execute the SQL statement (or multiple statements having the same fingerprint).
Count | The total number of times the SQL statement (or multiple statements having the same fingerprint) is executed. <br><br>The execution count is displayed in numerical value as well as in the form of a horizontal bar. The bar is color-coded to indicate the ratio of runtime success (indicated by blue) to runtime failure (indicated by red) of the execution count for the fingerprint. The bar also helps you compare the execution count across all SQL fingerprints in the table. <br><br>You can sort the table by count.
Mean Rows | The average number of rows returned or affected while executing the SQL statement (or multiple statements having the same fingerprint). <br><br>The number of mean rows is displayed in numerical value as well as in the form of a horizontal bar. The bar helps you compare the mean rows across all SQL fingerprints in the table. <br><br>You can sort the table by mean rows.
Mean Latency | The average service latency of the SQL statement (or multiple statements having the same fingerprint). <br><br> The mean latency is displayed in numerical value as well as in the form of a horizontal bar. The bar is color-coded to indicate the latency across the execution phases: parse (indicated by red), plan (indicated by yellow), execute (indicated by blue), and overhead (indicated by red). The bar also helps you compare the mean latencies across all SQL fingerprints in the table. <br><br>You can sort the table by mean latency.
Copy link
Contributor

Choose a reason for hiding this comment

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

Are there any docs on these phases that we can link to?

-----|------------
First Attempts | The cumulative number of first attempts to execute the SQL statement (or multiple statements having the same fingerprint).
Retries | The cumulative number of retries to execute the SQL statement (or multiple statements having the same fingerprint).
Max Retries |
Copy link
Contributor

Choose a reason for hiding this comment

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

tbd?

Parse | Red
Plan | Yellow
Run | Blue
Overhead | Red
Copy link
Contributor

Choose a reason for hiding this comment

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

There's also a row for "Overall", which in the alpha has the numbers but no bar.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Parameter | Description
-----|------------
Statement | The SQL statement or the fingerprint of similar SQL statements.
Time | The cumulative time taken to execute the SQL statement (or multiple statements having the same fingerprint).
Copy link
Contributor

Choose a reason for hiding this comment

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

For my part, repeating the parenthetical every time starts to feel excessive.

-----|------------
Total time | The cumulative time taken to execute the SQL statement (or multiple statements having the same fingerprint).
Execution count | The total number of times the SQL statement (or multiple statements having the same fingerprint) is executed.
Executed without retry | The total number of times the SQL statement (or multiple statements having the same fingerprint) is executed successfully on the first attempt.
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 the percentage of statements, not the total number.

Executed without retry | The total number of times the SQL statement (or multiple statements having the same fingerprint) is executed successfully on the first attempt.
Mean service latency | The average service latency of the SQL statement (or multiple statements having the same fingerprint).
Mean number of rows | The average number of rows returned or affected while executing the SQL statement (or multiple statements having the same fingerprint).

Copy link
Contributor

Choose a reason for hiding this comment

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

Are you planning on adding a section on the little table below that statistics box?

Copy link

@piyush-singh piyush-singh Jun 28, 2018

Choose a reason for hiding this comment

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

The table @couchand mentioned in his comments

image

@Amruta-Ranade
Copy link
Contributor Author

Review status: :shipit: complete! 0 of 0 LGTMs obtained


v2.1/admin-ui-statements-page.md, line 57 at r2 (raw file):

Previously, couchand (Andrew Couch) wrote…

For my part, repeating the parenthetical every time starts to feel excessive.

I know....I went back and forth on it, but decided to err on excessiveness instead of leaving it ambiguous. Will try to edit it another way.


v2.1/admin-ui-statements-page.md, line 60 at r2 (raw file):

Previously, couchand (Andrew Couch) wrote…

Are there any docs on these phases that we can link to?

Not sure..let me check.


v2.1/admin-ui-statements-page.md, line 62 at r2 (raw file):

Previously, couchand (Andrew Couch) wrote…

Does it make sense for this to be in the "Statement" row of the table above?

Agreed.


v2.1/admin-ui-statements-page.md, line 76 at r2 (raw file):

Previously, couchand (Andrew Couch) wrote…

tbd?

No...couldn't remember the confusing description we discussed in the meeting :) Need your help to understand it once more.


v2.1/admin-ui-statements-page.md, line 88 at r2 (raw file):

Previously, couchand (Andrew Couch) wrote…

There's also a row for "Overall", which in the alpha has the numbers but no bar.

Right. Will add it to the description.


v2.1/admin-ui-statements-page.md, line 102 at r2 (raw file):

Previously, couchand (Andrew Couch) wrote…

This is the percentage of statements, not the total number.

Huh!


v2.1/admin-ui-statements-page.md, line 105 at r2 (raw file):

Previously, couchand (Andrew Couch) wrote…

Are you planning on adding a section on the little table below that statistics box?

I wasn't sure if it's useful for the user (because it talks about DistSQL, which we generally decided is not user-useful). Also, not sure what "Failed?" meant: Did the app fail, the statement fail? So left it out till I figure that out. Can you help me with that?


Comments from Reviewable

Copy link

@piyush-singh piyush-singh left a comment

Choose a reason for hiding this comment

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

Agree with Andrew's comments, plus added a few of my own.

## Limitations

- If you have multiple applications running on the cluster, the **Statements** page shows cumulative parameter values across all applications, while the **Statements Details** page shows the values for the first application only.
- The **Statements** page provides the SQL statement details only for the [gateway node](architecture/sql-layer.html#overview). To view the details for other nodes, [access the Admin UI](admin-ui-access-and-navigate.html#access-the-admin-ui) from that node and navigate to `http://<node address>:8080/#/statements` from the browser.

Choose a reason for hiding this comment

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

I think the link to the architecture docs is a bit confusing here. Perhaps we can pull in the description of a gateway node to this doc while still having a link? So adding something like "...only for statements sent to the node to which the Admin UI is connected, also known as the gateway node." My construction is a bit awkward, but pulling more of the info onto the page would avoid the user needed to go elsewhere to understand what this means in practice.


- If you have multiple applications running on the cluster, the **Statements** page shows cumulative parameter values across all applications, while the **Statements Details** page shows the values for the first application only.
- The **Statements** page provides the SQL statement details only for the [gateway node](architecture/sql-layer.html#overview). To view the details for other nodes, [access the Admin UI](admin-ui-access-and-navigate.html#access-the-admin-ui) from that node and navigate to `http://<node address>:8080/#/statements` from the browser.
- The **Statements** page displays the details of the SQL statements executed only within a specified time interval. By default, the time interval is set to one hour; however, you can customize the interval using the [`diagnostics.reporting.interval`](cluster-settings.html#settings) cluster setting.

Choose a reason for hiding this comment

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

We should be really clear here: this is not a one hour moving window, but rather something that is completely reset once an hour so that at the end of the hour you will see 0 statements on this page after all of them are wiped.


### Parameters

The **Statements** page displays the execution time, count, and mean rows and latency for each statement fingerprint. By default, the statement fingerprints are sorted by their execution time; however, you can sort the table by count, mean rows, and mean latency.

Choose a reason for hiding this comment

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

These terms are defined below, but we use "execution time" here and "time" below; we should be consistent to make sure users understand what this metric is/what default sort behavior is.

Executed without retry | The total number of times the SQL statement (or multiple statements having the same fingerprint) is executed successfully on the first attempt.
Mean service latency | The average service latency of the SQL statement (or multiple statements having the same fingerprint).
Mean number of rows | The average number of rows returned or affected while executing the SQL statement (or multiple statements having the same fingerprint).

Copy link

@piyush-singh piyush-singh Jun 28, 2018

Choose a reason for hiding this comment

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

The table @couchand mentioned in his comments

image

@couchand
Copy link
Contributor

Reviewed 1 of 2 files at r1, 1 of 1 files at r2.
Review status: :shipit: complete! 0 of 0 LGTMs obtained


v2.1/admin-ui-statements-page.md, line 26 at r2 (raw file):

Previously, Amruta-Ranade (Amruta Ranade) wrote…

@couchand Hmmm....right. So effectively, even if the fingerprint is the statement itself, it is still a fingerprint. I think I can remove the "Whenever possible" part. Right?

Yeah, that's great.


v2.1/admin-ui-statements-page.md, line 57 at r2 (raw file):

Previously, Amruta-Ranade (Amruta Ranade) wrote…

I know....I went back and forth on it, but decided to err on excessiveness instead of leaving it ambiguous. Will try to edit it another way.

Fair enough.


v2.1/admin-ui-statements-page.md, line 76 at r2 (raw file):

Previously, Amruta-Ranade (Amruta Ranade) wrote…

No...couldn't remember the confusing description we discussed in the meeting :) Need your help to understand it once more.

"The most times that a single statement with this fingerprint needed to be retried."


v2.1/admin-ui-statements-page.md, line 77 at r2 (raw file):

Retries | The cumulative number of retries to execute the SQL statement (or multiple statements having the same fingerprint).
Max Retries |
Total |

The total number of executions of statements with this fingerprint. The sum of first attempts and cumulative retries.


Comments from Reviewable

@Amruta-Ranade
Copy link
Contributor Author

Review status: :shipit: complete! 0 of 0 LGTMs obtained


v2.1/admin-ui-statements-page.md, line 18 at r2 (raw file):

Previously, couchand (Andrew Couch) wrote…

It might be useful to link to documentation about setting the application, but as far as I can tell it's not really documented on it's own, just briefly on the cluster setting itself and then randomly mentioned elsewhere.

Done.


v2.1/admin-ui-statements-page.md, line 19 at r2 (raw file):

Previously, piyush-singh (Piyush Singh) wrote…

I think the link to the architecture docs is a bit confusing here. Perhaps we can pull in the description of a gateway node to this doc while still having a link? So adding something like "...only for statements sent to the node to which the Admin UI is connected, also known as the gateway node." My construction is a bit awkward, but pulling more of the info onto the page would avoid the user needed to go elsewhere to understand what this means in practice.

Done.


v2.1/admin-ui-statements-page.md, line 20 at r2 (raw file):

Previously, piyush-singh (Piyush Singh) wrote…

We should be really clear here: this is not a one hour moving window, but rather something that is completely reset once an hour so that at the end of the hour you will see 0 statements on this page after all of them are wiped.

Done.


v2.1/admin-ui-statements-page.md, line 26 at r2 (raw file):

Previously, couchand (Andrew Couch) wrote…

Yeah, that's great.

Done.


v2.1/admin-ui-statements-page.md, line 46 at r2 (raw file):

Previously, couchand (Andrew Couch) wrote…

I've opened cockroachdb/cockroach#27052 to track the idea to use placeholders for fingerprints. Pending the way that gets resolved these examples may move up to the block above!

👍


v2.1/admin-ui-statements-page.md, line 50 at r2 (raw file):

Previously, piyush-singh (Piyush Singh) wrote…

These terms are defined below, but we use "execution time" here and "time" below; we should be consistent to make sure users understand what this metric is/what default sort behavior is.

Yeah...I was trying to give more context about the parameters in the description, but I agree it's better to be consistent with the terms.


v2.1/admin-ui-statements-page.md, line 62 at r2 (raw file):

Previously, Amruta-Ranade (Amruta Ranade) wrote…

Agreed.

Done.


v2.1/admin-ui-statements-page.md, line 76 at r2 (raw file):

Previously, couchand (Andrew Couch) wrote…

"The most times that a single statement with this fingerprint needed to be retried."

Done.


v2.1/admin-ui-statements-page.md, line 77 at r2 (raw file):

Previously, couchand (Andrew Couch) wrote…

The total number of executions of statements with this fingerprint. The sum of first attempts and cumulative retries.

Done.


v2.1/admin-ui-statements-page.md, line 88 at r2 (raw file):

Previously, Amruta-Ranade (Amruta Ranade) wrote…

Right. Will add it to the description.

Done.


v2.1/admin-ui-statements-page.md, line 102 at r2 (raw file):

Previously, Amruta-Ranade (Amruta Ranade) wrote…

Huh!

Done.


v2.1/admin-ui-statements-page.md, line 105 at r2 (raw file):

Previously, piyush-singh (Piyush Singh) wrote…

The table @couchand mentioned in his comments

image

Done.


Comments from Reviewable

@Amruta-Ranade Amruta-Ranade requested a review from lnhsingh June 28, 2018 17:50
@lnhsingh
Copy link
Contributor

:lgtm: with some nits


Review status: :shipit: complete! 0 of 0 LGTMs obtained (and 1 stale)


v2.1/admin-ui-statements-page.md, line 6 at r3 (raw file):

---

The **Statements** page helps you identify the frequently executed or high latency [SQL statements](sql-statements.html). The **Statements** page also allows you to drill down to the details of an individual SQL statement by clicking on the statement to view the **Statement Details** page.

nit: "drill down" is a colloquialism. Suggestion: "[...] page also allows you to view the details [...]"


v2.1/admin-ui-statements-page.md, line 18 at r3 (raw file):

## Limitations

- If you have multiple applications running on the cluster, the **Statements** page shows the statements from all of the applications; however, there is no way to map the statements to the applications. Also, the **Statements Details** page shows the values only for the application specified by the [`application_name`](https://www.cockroachlabs.com/docs/dev/show-vars.html#supported-variables) session setting. The next CockroachDB alpha release allows you to choose the application on the **Statements** page and, by extension, the **Statement Details** page.

nit: "The next CockroachDB alpha release allows you to choose the application " -> "In the next CockroachDB alpha release, you will be able to choose the application"

Side note/ question: Is it ok to mention what a future alpha release may include? What happens if it's not included? (This might not be a big deal)


v2.1/admin-ui-statements-page.md, line 20 at r3 (raw file):

- If you have multiple applications running on the cluster, the **Statements** page shows the statements from all of the applications; however, there is no way to map the statements to the applications. Also, the **Statements Details** page shows the values only for the application specified by the [`application_name`](https://www.cockroachlabs.com/docs/dev/show-vars.html#supported-variables) session setting. The next CockroachDB alpha release allows you to choose the application on the **Statements** page and, by extension, the **Statement Details** page.
- The **Statements** page provides the SQL statement details only for statements sent to the node from which the Admin UI is accessed (that is, the [gateway node](architecture/sql-layer.html#overview)). To view the details for other nodes, [access the Admin UI](admin-ui-access-and-navigate.html#access-the-admin-ui) from that node and navigate to `http://<node address>:8080/#/statements` from the browser.
- The **Statements** page displays the details of the SQL statements executed only within a specified time interval. At the end of the specified time interval, the display is wiped clean, and you'll not see any statements on the **Statements** page till the next set of statements is executed. By default, the time interval is set to one hour; however, you can customize the interval using the [`diagnostics.reporting.interval`](cluster-settings.html#settings) cluster setting.

nit: "till" -> "until"


v2.1/admin-ui-statements-page.md, line 32 at r3 (raw file):

A statement fingerprint is generated when two or more statements have the same abstractions. For example, the following statements have the same abstractions:

`INSERT INTO new_order(product_id, customer_id, transaction_id) VALUES (380, 11, 11098)`

Should this be a bulleted list?


v2.1/admin-ui-statements-page.md, line 36 at r3 (raw file):

`INSERT INTO new_order(product_id, customer_id, transaction_id) VALUES (784, 452, 78)`

Thus they can have the same fingerprint:

nit: "Thus, they [...]"


v2.1/admin-ui-statements-page.md, line 42 at r3 (raw file):

The following statements are different enough to not have the same fingerprint:

`INSERT INTO orders(product_id, customer_id, transaction_id) VALUES (380, 11, 11098)`

Bullets?


v2.1/admin-ui-statements-page.md, line 58 at r3 (raw file):

Statement | The SQL statement or the fingerprint of similar SQL statements.<br><br>To view additional details of a statement fingerprint, click on the statement fingerprint in the **Statement** column to see the [**Statement Details** page](#statement-details-page).
Time | The cumulative time taken to execute the SQL statement (or multiple statements having the same fingerprint).
Count | The total number of times the SQL statement (or multiple statements having the same fingerprint) is executed. <br><br>The execution count is displayed in numerical value as well as in the form of a horizontal bar. The bar is color-coded to indicate the ratio of runtime success (indicated by blue) to runtime failure (indicated by red) of the execution count for the fingerprint. The bar also helps you compare the execution count across all SQL fingerprints in the table. <br><br>You can sort the table by count.

Nothing to really change here, but maybe something to think about in terms of a style choice: for color-coded things, do you think we should use colors? For example, instead of having a parenthetical (e.g., "(indicated by blue)"), we could just say "...indicate the ratio of runtime success", where "runtime success" is the color blue.

Actually, now that I think about it more, that might make our docs less accessible (in terms of colorblindness). Never mind me :) (but maybe this is something to discuss later?)


v2.1/admin-ui-statements-page.md, line 66 at r3 (raw file):

The **Statement Details** page displays the details of the execution count, latency by phase, row count, and statistics for the selected statement fingerprint.

### Execution count

Should all of these H3s under Statement Details page be title case since they are the names of the tables?


v2.1/admin-ui-statements-page.md, line 88 at r3 (raw file):

ember the confusing description we discussed in the meeting :) Need your help to understand it once more.


Comments from Reviewable

@Amruta-Ranade
Copy link
Contributor Author

Review status: :shipit: complete! 1 of 0 LGTMs obtained


v2.1/admin-ui-statements-page.md, line 18 at r3 (raw file):

Previously, lhirata wrote…

nit: "The next CockroachDB alpha release allows you to choose the application " -> "In the next CockroachDB alpha release, you will be able to choose the application"

Side note/ question: Is it ok to mention what a future alpha release may include? What happens if it's not included? (This might not be a big deal)

The change is already checked in, but didn't make it to this alpha. So in this case, I assume it is safe to say the limitation will be removed.


v2.1/admin-ui-statements-page.md, line 32 at r3 (raw file):

Previously, lhirata wrote…

Should this be a bulleted list?

Done.


v2.1/admin-ui-statements-page.md, line 42 at r3 (raw file):

Previously, lhirata wrote…

Bullets?

Done.


v2.1/admin-ui-statements-page.md, line 58 at r3 (raw file):

Previously, lhirata wrote…

Nothing to really change here, but maybe something to think about in terms of a style choice: for color-coded things, do you think we should use colors? For example, instead of having a parenthetical (e.g., "(indicated by blue)"), we could just say "...indicate the ratio of runtime success", where "runtime success" is the color blue.

Actually, now that I think about it more, that might make our docs less accessible (in terms of colorblindness). Never mind me :) (but maybe this is something to discuss later?)

Oooh..that's an interesting point! Let's discuss this at the next Docs meeting (pretty sure it applies elsewhere too. Worth a holistic discussion.)


v2.1/admin-ui-statements-page.md, line 66 at r3 (raw file):

Previously, lhirata wrote…

Should all of these H3s under Statement Details page be title case since they are the names of the tables?

Thank you! I spent a lot of time deciding the right thing to do here. Thanks for the definitive answer :)


Comments from Reviewable

@piyush-singh
Copy link

Review status: :shipit: complete! 1 of 0 LGTMs obtained


v2.1/admin-ui-statements-page.md, line 18 at r3 (raw file):

Previously, lhirata wrote…

nit: "The next CockroachDB alpha release allows you to choose the application " -> "In the next CockroachDB alpha release, you will be able to choose the application"

Side note/ question: Is it ok to mention what a future alpha release may include? What happens if it's not included? (This might not be a big deal)

I agree that we should be careful here, since we may end up indicating a feature is coming that might slip. In this specific case, we are referencing a change that is already/soon to be merged to Master but did not make the cutoff for the 7/2 Alpha, so it definitely will be in the 7/30 Alpha. In general, I think we should only reference changes in future Alphas if they are already merged to Master at the time of publishing the documentation (I'd check with Andrew on status for this).


Comments from Reviewable


- If you have multiple applications running on the cluster, the **Statements** page shows cumulative parameter values across all applications, while the **Statements Details** page shows the values for the first application only.
- The **Statements** page provides the SQL statement details only for the [gateway node](architecture/sql-layer.html#overview). To view the details for other nodes, [access the Admin UI](admin-ui-access-and-navigate.html#access-the-admin-ui) from that node and navigate to `http://<node address>:8080/#/statements` from the browser.
- The **Statements** page displays the details of the SQL statements executed only within a specified time interval. By default, the time interval is set to one hour; however, you can customize the interval using the ` ` cluster setting.
Copy link
Contributor

Choose a reason for hiding this comment

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

missing the actual cluster setting


A statement fingerprint is a grouping of similar SQL statements in their abstracted form by replacing the parameter values with `_`. Grouping similar SQL statements as fingerprints helps you quickly identify the frequently executed SQL statements and their latencies.

A statement fingerprint is generated when two or more statements have the same abstractions. For example, the following statements have the same abstractions:
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe "A statement fingerprint is generated when two or more statements are the same after any literal values in them (e.g. numbers and strings) are replaced with underscores. For example, the following statements have the same once their numbers have been replaced with underscores"


The **Statements** page displays the details of SQL statement fingerprints instead of individual SQL statements.

A statement fingerprint is a grouping of similar SQL statements in their abstracted form by replacing the parameter values with `_`. Grouping similar SQL statements as fingerprints helps you quickly identify the frequently executed SQL statements and their latencies.
Copy link
Contributor

Choose a reason for hiding this comment

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

Replace "parameter values" with "literal values". Not clear what "parameter" means in this context.

Copy link
Contributor

@vilterp vilterp left a comment

Choose a reason for hiding this comment

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

Great, thanks!

@Amruta-Ranade
Copy link
Contributor Author

Update: PR reviewed by Lauren, Andrew, Pete, Piyush.
To-do: @jseldess 's review. As discussed in this week's 1:1, merging this PR now and will open a follow-up PR for Jesse's review.

@Amruta-Ranade Amruta-Ranade changed the title [WIP]: Admin UI Statements page - Reference doc Admin UI Statements page - Reference doc Jun 28, 2018
@Amruta-Ranade Amruta-Ranade merged commit 46aa678 into master Jun 28, 2018
@Amruta-Ranade Amruta-Ranade deleted the query_screen_doc branch June 28, 2018 21:48
Copy link
Contributor

@jseldess jseldess left a comment

Choose a reason for hiding this comment

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

Nice! Just a few comments for later follow-up.

@@ -0,0 +1,116 @@
---
title: Statements Page
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add a summary: field to the front-matter. That's used as the page's meta description for SEO.


A statement fingerprint is a grouping of similar SQL statements in their abstracted form by replacing the literal values with underscores (`_`). Grouping similar SQL statements as fingerprints helps you quickly identify the frequently executed SQL statements and their latencies.

A statement fingerprint is generated when two or more statements are the same after any literal values in them (e.g. numbers and strings) are replaced with underscores. For example, the following statements have the same once their numbers have been replaced with underscores:
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add a comma after e.g..


A statement fingerprint is generated when two or more statements are the same after any literal values in them (e.g. numbers and strings) are replaced with underscores. For example, the following statements have the same once their numbers have been replaced with underscores:

- `INSERT INTO new_order(product_id, customer_id, transaction_id) VALUES (380, 11, 11098)`
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Add space between table name and parentheses with column list in this and subsequent statements.


Parameter | Description
-----|------------
Statement | The SQL statement or the fingerprint of similar SQL statements.<br><br>To view additional details of a statement fingerprint, click on the statement fingerprint in the **Statement** column to see the [**Statement Details** page](#statement-details-page).
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I don't think you need to say "in the Statement column" since this is a description of that column.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hm, this col should probably be called "statement fingerprint" 🤔

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 agree.

Parameter | Description
-----|------------
Statement | The SQL statement or the fingerprint of similar SQL statements.<br><br>To view additional details of a statement fingerprint, click on the statement fingerprint in the **Statement** column to see the [**Statement Details** page](#statement-details-page).
Time | The cumulative time taken to execute the SQL statement (or multiple statements having the same fingerprint).
Copy link
Contributor

@jseldess jseldess Jun 28, 2018

Choose a reason for hiding this comment

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

This description leaves me a little confused:

  • If it's a fingerprint, does Time tell you the cumulative time for all of the statements grouped as the fingerprint, or for each individual statement? If the former, is it an average?

Copy link
Contributor

Choose a reason for hiding this comment

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

Since some of the other fields mention "average", I'm wondering if that's really the case here, too?

Copy link
Contributor

Choose a reason for hiding this comment

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

We should rename this column to "total time" or "cumulative time". It's average time * count (both already columns in their own right).

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, I wouldn't say "the SQL statement (or multiple statements having the same fingerprint)" — it's always a fingerprint, it's just that sometimes there's only one statement with that fingerprint.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree and think that would remove some of my confusion. We define fingerprint above, so we can just use that term, I think.

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 definitely want to revisit the phrasing later. The reason I worded it this way was because "execute the statement fingerprint" felt weird because we don't execute the fingerprint, but multiple SQL statements with the same fingerprint. Can't think of a way to convey that without stating it explicitly. Suggestions welcome :)

Copy link
Contributor

Choose a reason for hiding this comment

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

I see what you mean. Maybe:

The cumulative time taken to execute the SQL statements represented by the fingerprint.

@vilterp, when you say that average time is already a column, do you mean Mean Latency?

Copy link
Contributor

Choose a reason for hiding this comment

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

Correct, Mean Latency. We're just doing users a favor by multiplying two columns already on the page.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah..I think the docs need to be more explicit in explaining that to the users. Will work on that in the follow-up PR.


Parameter | Description
-----|------------
First Attempts | The cumulative number of first attempts to execute the SQL statement (or multiple statements having the same fingerprint).
Copy link
Contributor

Choose a reason for hiding this comment

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

Again, the term "cumulative" here and below feels a little confusing, perhaps because of the way the rest of the sentence is phrased. What does that term really add?

@vilterp
Copy link
Contributor

vilterp commented Jun 28, 2018

Would it be helpful to introduce two terms, "fingerprint" and "fingerprint group"?

E.g. for the queries in the example, this is the fingerprint:

INSERT INTO new_order(product_id, customer_id, no_w_id) VALUES (_, _, _)

and this set of statements with the same fingerprint is the fingerprint group:

  • INSERT INTO orders(product_id, customer_id, transaction_id) VALUES (380, 11, 11098)
  • INSERT INTO new_order(product_id, customer_id, transaction_id) VALUES (380, 11, 11098)
  • INSERT INTO new_order(product_id, customer_id, transaction_id) VALUES ($1, 11, 11098)
  • INSERT INTO new_order(product_id, customer_id, transaction_id) VALUES ($1, $2, 11098)
  • ...

The current phrasing conflates them a bit, usually using "fingerprint" as what I am calling "fingerprint group". I just think it's more precise to say that a statement's fingerprint is the statement with literals replaced with underscores, and we group statements by their fingerprint, forming a set of fingerprint groups, for each of which we show aggregate stats.

@Amruta-Ranade
Copy link
Contributor Author

AFAIK, it's not the fingerprint group, it's the statements' group. Fingerprint group would imply grouping of distinct fingerprints IMO.

@vilterp
Copy link
Contributor

vilterp commented Jun 28, 2018

Yeah. It's a group of statements which have the same fingerprint. Not sure how to best abbreviate that.

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.

7 participants