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

release-19.1: ui: adjust QPS Summary to match Time Series and Node Map values. #35905

Merged
merged 1 commit into from
Mar 19, 2019

Conversation

celiala
Copy link
Collaborator

@celiala celiala commented Mar 18, 2019

Backport 1/1 commits from #35693.

/cc @cockroachdb/release


In #283 and #23967,
we note three QPS-related areas in the Admin UI that were causing confusion
for users:

  • The SQL Queries Metric, which shows QPS data for
    SELECT/INSERT/UPDATE/DELETE SQL queries.
  • The Node Map, which shows QPS data for
    SELECT/INSERT/UPDATE/DELETE SQL queries.
  • The 'Queries per second' in the Summary Bar, which calcuates QPS for all
    SQL queries (SELECT/INSERT/UPDATE/DELETE, but also includes DDL statements
    and transaction boundaries).

This commit:

  • Updates the 'Queries per second' in the Summary Bar to just
    summarize the query types displayed in SQL Queries Time Series Metric and
    Node Map
  • Clarifies which queries are included in Summary Bar with summary stat
    message.
  • Removes P50 latency metric, since:
    • latency metrics still include all queries and this change might
      introduce new user questions/confusion.
    • our support team has confirmed that while users care about P99 and P90,
      we actually don't see customers concerned about P50.
    • [Why P50 but not P90] For the P90 & P99 metrics, a push up will most
      likely be from normal SELECT/INSERT/UPDATE/DELETEs. For 50, however,
      if clients are doing a large number of complex transactions or SET
      commands this could really skew the P50 result.

While this doesn't fully address all the concerns raised from #283 and #23967,
this at least provides a short-term fix of making all the numbers consistent.

Time Series vs Summary Bar. Before (discrepancy of 142.9):
adriatic off-by-n

Time Series vs Summary Bar. After (sums match up):
adriatic matches

Node Map vs Summary Bar. After (sums match up):
adriatic after node-map

Updated Summary Bar:
updated summary bar

Release note (admin ui change): 'Queries per second' metric in Summary Bar
adjusted to only summarize the query types displayed in SQL Queries Time
Series Metric and Node Map.

In [cockroachdb#283](cockroachlabs/support#283) and cockroachdb#23967,
we note three QPS-related areas in the Admin UI that were causing confusion
for users:

- The SQL Queries Metric, which shows QPS data for
  `SELECT/INSERT/UPDATE/DELETE` SQL queries
- The Node Map, which shows QPS data for
  `SELECT/INSERT/UPDATE/DELETE` SQL queries
- The 'Queries per second' in the Summary Bar, which calculates QPS for all
  SQL queries (`SELECT/INSERT/UPDATE/DELETE`, in addition to DDL statements
  and transaction boundaries)

This commit:

- Updates the 'Queries per second' in the Summary Bar to just
  summarize the query types displayed in SQL Queries Time Series Metric and
  Node Map
- Clarifies which queries are included in Summary Bar with a message
  right below the stat (`summaryStatMessage`)
- Removes P50 latency metric in Summary Bar, since:
  - Latency metrics still include all queries, and this change might
    introduce new user questions/confusion
  - Our support team has confirmed that while users care about P99 and P90,
    we actually don't see customers concerned about P50.
  - [Why remove P50 but not P90] For the P90 & P99 metrics, a push up will
    most likely be from normal SELECT/INSERT/UPDATE/DELETEs. For 50, however,
    if clients are doing a large number of complex transactions or SET
    commands this could really skew the P50 result.

While this commit doesn't fully address all the concerns raised from cockroachdb#283
and cockroachdb#23967, this at least provides a short-term fix of making existing
visible QPS metrics consistent.

Time Series vs Summary Bar. Before (discrepancy of 142.9):
<img width="400" alt="adriatic off-by-n" src="https://user-images.githubusercontent.com/3051672/54290723-db120600-4581-11e9-878d-dccd771848fa.png">

Time Series vs Summary Bar. After (sums match up):
<img width="400" alt="adriatic matches" src="https://user-images.githubusercontent.com/3051672/54290716-d6e5e880-4581-11e9-8d04-22a3df639265.png">

Node Map vs Summary Bar. After (sums match up):
<img width="400" alt="adriatic after node-map" src="https://user-images.githubusercontent.com/3051672/54292511-b79c8a80-4584-11e9-9c5b-087e65fa765a.png">

Updated Summary Bar:
<img width="400" alt="updated summary bar" src="https://user-images.githubusercontent.com/3051672/54309359-5b4a6280-45a6-11e9-8fb3-f0850bc3d44e.png">

Release note (admin ui change): 'Queries per second' metric in Summary Bar
adjusted to only summarize the query types displayed in SQL Queries Time
Series Metric and Node Map.
@celiala celiala requested review from knz and a team March 18, 2019 19:50
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@knz
Copy link
Contributor

knz commented Mar 19, 2019

this will get a green build when #35939 merges. it's green now

@celiala
Copy link
Collaborator Author

celiala commented Mar 19, 2019

bors r+

@celiala
Copy link
Collaborator Author

celiala commented Mar 19, 2019

just remembered:

we use the github green merge button instead of bors.

bors r-

@craig
Copy link
Contributor

craig bot commented Mar 19, 2019

Canceled

@celiala celiala merged commit 5784af8 into cockroachdb:release-19.1 Mar 19, 2019
@celiala celiala deleted the backport19.1-35693 branch March 19, 2019 15:23
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.

4 participants