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

The DB console is treating two different transactions as a single fingerprint #65106

Closed
kevin-v-ngo opened this issue May 13, 2021 · 2 comments
Closed
Assignees
Labels
A-sql-console-general SQL Observability issues on the DB console spanning multiple areas. Includes Cockroach Cloud Console C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior.

Comments

@kevin-v-ngo
Copy link

kevin-v-ngo commented May 13, 2021

The DB console transaction page treats two different transactions as the same fingerprint even though they have different counts of the same statement fingerprint.

Repro
Run the following:

  CREATE TABLE debug (id INT, name VARCHAR);
  
  BEGIN;
  INSERT INTO debug values(1, 'a');
  COMMIT;
  
  BEGIN;
  INSERT INTO debug values (1, 'a');
  INSERT INTO debug values(1, 'b');
  COMMIT;

Go to the DB console Transactions page and search for 'debug'. You expect to see two rows but there is 1 row. This single row also shows that the transaction fingerprint has an Execution Count of 3.

I suspect that because these transactions contain all the same statement fingerprint, we are treating it as a single transaction fingerprint; however, this doesn't explain why the Execution Count is 3. I would have expected two rows in the console page where each row has an Execution Count of 1.

Example:

T1: S1 (Read), S2 (Write)
T2: S2 (Write), S1 (Read)
T3: S2 (Write), S1 (Read), S2 (Write), S2 (Write)

Issue

All three transactions are treated as the same transaction fingerprint because they all have the same statement fingerprints (S1 and S2).

Improved behavior

We should treat them as separate transaction fingerprints because they can have differences in runtime statistics just based on ordering and number of their statements.

T3 should be different because the user can execute a transaction with a high number of statements of the same fingerprint and it can be aggregated with another transaction (like T1 or T2) with just a few statements of the same fingerprint.

T2 and T1 should be different fingerprints because the user can submit a statement dependent on the previous statement within the transaction. S1 can wait on S2 in T2 if it is reading from the same row S2 wrote. This dependency does not exist in T1.

For T3 scenario, we should consider updating the UX to avoid showing all statements. For instance, just show the counts S2(1x), S1(1x), S2(2x).

Epic: CRDB-9867

@kevin-v-ngo kevin-v-ngo added C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. T-sql-observability labels May 13, 2021
@kevin-v-ngo kevin-v-ngo added the A-sql-console-general SQL Observability issues on the DB console spanning multiple areas. Includes Cockroach Cloud Console label May 21, 2021
@maryliag maryliag self-assigned this May 26, 2021
maryliag added a commit to maryliag/cockroach that referenced this issue Jun 18, 2021
Transactions that cointained the same statements (independent
of their order or execution count) were being considerate as the same
fingerprint. This PR addresses this issue by creating a fingerprint
that includes all statement in order of their execution and with
their repetition count.

A statement key was made by the query, database and if was part of
an implicit txn, meaning all statements part of a transaction (not
necessarily transactions with the same fingerprint) were bring grouped
on the Transaction Details page. To solve this issue, the txnID was
introduced as part of a statement key, which made each statement that
was part of an explicit transaction to be considerate having their
own unique fingerprint. Then when this information is getting passed
on to the frontend, we grouped the statements part of the same
transaction fingerprint. When the flush to disc function gets created,
this change has to be considerate and the aggregated statements
should be flushed, not the initial in-memory value that are not yet
grouped.

Fixes: cockroachdb#65106

Release note (bug fix): Differentiate Transaction based on their
fingerprint (made by their query, database, app and complete list of
statements execution in order) and display the complete Transaction
fingerprint into Transaction Details page.
@maryliag maryliag assigned Azhng and unassigned maryliag Sep 13, 2021
Azhng added a commit to Azhng/cockroach that referenced this issue Sep 16, 2021
fingerprintID

Previously, statement that are executed as part of different
transactions are grouped together resulting in one single
stats entry. This results in weird UX and makes it difficult
for developer to debug slow queries.
This commit groups statement statsitics based on its transaction
fingerprintIDs on top of the statement fingerprint ID.

Partially addresses cockroachdb#65106, cockroachdb#59205

Release note (bug fix): statement statsitics are now grouped by
the statement's corresponding transaction fingerprints.
Azhng added a commit to Azhng/cockroach that referenced this issue Sep 27, 2021
fingerprintID

Previously, statement that are executed as part of different
transactions are grouped together resulting in one single
stats entry. This results in weird UX and makes it difficult
for developer to debug slow queries.
This commit groups statement statsitics based on its transaction
fingerprintIDs on top of the statement fingerprint ID.

Partially addresses cockroachdb#65106, cockroachdb#59205

Release note (bug fix): statement statsitics are now grouped by
the statement's corresponding transaction fingerprints.
Azhng added a commit to Azhng/cockroach that referenced this issue Oct 6, 2021
fingerprintID

Previously, statement that are executed as part of different
transactions are grouped together resulting in one single
stats entry. This results in weird UX and makes it difficult
for developer to debug slow queries.
This commit groups statement statsitics based on its transaction
fingerprintIDs on top of the statement fingerprint ID.

Partially addresses cockroachdb#65106, cockroachdb#59205

Release note (bug fix): statement statsitics are now grouped by
the statement's corresponding transaction fingerprints.
Azhng added a commit to Azhng/cockroach that referenced this issue Oct 6, 2021
fingerprintID

Previously, statement that are executed as part of different
transactions are grouped together resulting in one single
stats entry. This results in weird UX and makes it difficult
for developer to debug slow queries.
This commit groups statement statsitics based on its transaction
fingerprintIDs on top of the statement fingerprint ID.

Partially addresses cockroachdb#65106, cockroachdb#59205

Release note (bug fix): statement statsitics are now grouped by
the statement's corresponding transaction fingerprints.
Azhng added a commit to Azhng/cockroach that referenced this issue Oct 7, 2021
fingerprintID

Previously, statement that are executed as part of different
transactions are grouped together resulting in one single
stats entry. This results in weird UX and makes it difficult
for developer to debug slow queries.
This commit groups statement statsitics based on its transaction
fingerprintIDs on top of the statement fingerprint ID.

Partially addresses cockroachdb#65106, cockroachdb#59205

Release note (bug fix): statement statsitics are now grouped by
the statement's corresponding transaction fingerprints.
craig bot pushed a commit that referenced this issue Oct 7, 2021
70261: sql: group statement statistics based on its transaction fingerprintID r=matthewtodd a=Azhng

Depends on #69710

# First Commit

sql: plumb sqlstats.TestingKnobs down into ssmemstorage

This commit plumbs sqlstats.TestingKnobs into ssmemstorage
in order to allow later unit tests to manipulate times in the
tests.

Release note: None


# Second Commit

sql: remove GetStatementStats and GetTransactionStats API

Previsously, we had GetStatementStats and GetTransactionStats
exported as part of the interface for SQL Stats subsystem.
These two methods are only used within tests and nowhere else
in the code base.
This commit removes those two exported methods and rewrite the
tests to use the iterator API.

Release note: None

# Third Commit

sql: group statement statistics based on its transaction fingerprintID

Previously, statement that are executed as part of different
transactions are grouped together resulting in one single
stats entry. This results in weird UX and makes it difficult
for developer to debug slow queries.
This commit groups statement statsitics based on its transaction
fingerprintIDs on top of the statement fingerprint ID.

Partially addresses #65106, #59205

Release note (bug fix): statement statsitics are now grouped by
the statement's corresponding transaction fingerprints.

Co-authored-by: Azhng <[email protected]>
ericharmeling pushed a commit to ericharmeling/cockroach that referenced this issue Oct 20, 2021
fingerprintID

Previously, statement that are executed as part of different
transactions are grouped together resulting in one single
stats entry. This results in weird UX and makes it difficult
for developer to debug slow queries.
This commit groups statement statsitics based on its transaction
fingerprintIDs on top of the statement fingerprint ID.

Partially addresses cockroachdb#65106, cockroachdb#59205

Release note (bug fix): statement statsitics are now grouped by
the statement's corresponding transaction fingerprints.
Azhng added a commit to Azhng/cockroach that referenced this issue Oct 26, 2021
transaction fingerprint ID

Previously, transactionDetail page shows statement statistics aggregated
across **all** executions of that statement fingerprint. This led to
confusing UX and the statement statistics were subsequently removed
from transactionDetail page.
This commit reintroduces statement statistics on transactionDetail
page. The statistics now are only aggregated across the statement
fingerprints that are part of the selected transaction fingerprint.

Resolves cockroachdb#59205 cockroachdb#65106

Release note (ui change): transactionDetail page now shows statement
statistics scoped by the transaction fingeprint.
Azhng added a commit to Azhng/cockroach that referenced this issue Oct 26, 2021
…d by

transaction fingerprint ID

Previously, the transactionDetail page showed statement statistics aggregated
across **all** executions of that statement fingerprint. This led to
confusing UX and the statement statistics were subsequently removed
from transactionDetail page.
This commit reintroduces statement statistics on the transactionDetail
page. The statistics now are only aggregated across the statement
fingerprints that are part of the selected transaction fingerprint.

Resolves cockroachdb#59205 cockroachdb#65106

Release note (ui change): transactionDetail page now shows statement
statistics scoped by the transaction fingeprint.
craig bot pushed a commit that referenced this issue Oct 26, 2021
71543: roachtest: disable prometheus for scrub r=otan a=otan

I'm unable to reproduce the error running roachtest myself, and it's the
only set of tests affected.

Resolves #71979

Release note: None

71927: sql/catalog/nstree: optimize with lazy initialization, pooling btrees r=ajwerner a=ajwerner

This change comes in response to
#66112 (comment).

The thrust of the change is to lazily initialize the data structures so that
when they are not used, they do not incur cost. Additionally, pool the
underlying tree so that when they are used, the allocations are effectively
free. This was originally deemed unimportant because the connExecutor
descs.Collection is long-lived. Unfortunately, others, as used for the
type resolve in distsql, are not.

```
name                                           old time/op    new time/op    delta
FlowSetup/vectorize=true/distribute=true-16       145µs ± 2%     139µs ± 3%   -3.94%  (p=0.000 n=19+20)
FlowSetup/vectorize=true/distribute=false-16      141µs ± 3%     134µs ± 2%   -4.66%  (p=0.000 n=18+19)
FlowSetup/vectorize=false/distribute=true-16      138µs ± 4%     132µs ± 4%   -4.23%  (p=0.000 n=20+20)
FlowSetup/vectorize=false/distribute=false-16     133µs ± 3%     127µs ± 3%   -4.41%  (p=0.000 n=20+19)

name                                           old alloc/op   new alloc/op   delta
FlowSetup/vectorize=true/distribute=true-16      38.1kB ± 3%    36.7kB ± 2%   -3.58%  (p=0.000 n=18+18)
FlowSetup/vectorize=true/distribute=false-16     36.2kB ± 0%    34.9kB ± 0%   -3.66%  (p=0.000 n=18+16)
FlowSetup/vectorize=false/distribute=true-16     42.6kB ± 0%    41.3kB ± 0%   -3.11%  (p=0.000 n=17+17)
FlowSetup/vectorize=false/distribute=false-16    41.0kB ± 0%    39.7kB ± 0%   -3.22%  (p=0.000 n=17+17)

name                                           old allocs/op  new allocs/op  delta
FlowSetup/vectorize=true/distribute=true-16         368 ± 0%       314 ± 0%  -14.67%  (p=0.000 n=16+17)
FlowSetup/vectorize=true/distribute=false-16        354 ± 0%       300 ± 0%  -15.25%  (p=0.000 n=17+17)
FlowSetup/vectorize=false/distribute=true-16        338 ± 1%       283 ± 1%  -16.13%  (p=0.000 n=19+19)
FlowSetup/vectorize=false/distribute=false-16       325 ± 0%       271 ± 0%  -16.62%  (p=0.000 n=18+18)
```

Omitting a release note because I'm doubtful this meaningfully shows up on its
own.

Release note: None

71960: ui: Added informative warning for insufficient privileges r=nathanstilwell a=nathanstilwell

<img width="972" alt="Screen Shot 2021-10-25 at 17 17 49" src="https://user-images.githubusercontent.com/397448/138772352-117456e1-10a8-475b-9b1c-79b0e2327558.png">

Previously, `/#/reports/nodes` would seem to hang in a loading state
indefinitely when a DB Console user wouldn't have admin privileges for
that endpoint. This was due to nodes data being empty from a 403
response.

An error is captured in the application state for this response, so
mapping this error as a prop to the component, the UI can distinguish
between the nodes data simply being empty and being empty due to a
restriction error.

<img width="827" alt="Screen Shot 2021-10-25 at 17 16 07" src="https://user-images.githubusercontent.com/397448/138772135-33321d34-9728-489b-8549-1c8bd55cf650.png">

Detecting this error, we render an `<InlineAlert />` to notify the user
of their lack of privileges.

Release note (ui change): All Nodes report notifies user is they need
more privileges to view information.

71966: ui: default filter on Transaction and Statement pages now exclude internals r=maryliag a=maryliag

Previously, the default value when no App filter was
selected on Transactions and Statements page, we were showing
all data, now we're excluding internal transaction/statements.

Fixes #70544

Release note (ui change): The default value when no App is selected
on Transactions and Statements filter is now excluding internal
Transactions and Statements.

71967: server,ui: transactionDetail page now shows statement stats scoped by transaction fingerprint ID r=matthewtodd a=Azhng

Previously, transactionDetail page shows statement statistics aggregated
across **all** executions of that statement fingerprint. This led to
confusing UX and the statement statistics were subsequently removed
from transactionDetail page.
This commit reintroduces statement statistics on transactionDetail
page. The statistics now are only aggregated across the statement
fingerprints that are part of the selected transaction fingerprint.

Resolves #59205 #65106

Release note (ui change): transactionDetail page now shows statement
statistics scoped by the transaction fingeprint.



https://user-images.githubusercontent.com/9267198/138916095-c91895d5-6f34-49b7-87dd-81f375b3f582.mov



71987: sql: skip TestSavepoints r=nkodali a=jbowens

Refs: #70220

Reason: flaky test

Generated by bin/skip-test.

Release justification: non-production code changes

Release note: None

Co-authored-by: Oliver Tan <[email protected]>
Co-authored-by: Andrew Werner <[email protected]>
Co-authored-by: Nathan Stilwell <[email protected]>
Co-authored-by: Marylia Gutierrez <[email protected]>
Co-authored-by: Azhng <[email protected]>
Co-authored-by: Jackson Owens <[email protected]>
@maryliag
Copy link
Contributor

@Azhng can this be closed?

@Azhng
Copy link
Contributor

Azhng commented Oct 27, 2021

Ah yes this can be closed. Seems like I can't put two issues on the single Resolves line

@Azhng Azhng closed this as completed Oct 27, 2021
Azhng added a commit to Azhng/cockroach that referenced this issue Nov 18, 2021
fingerprintID

Previously, statement that are executed as part of different
transactions are grouped together resulting in one single
stats entry. This results in weird UX and makes it difficult
for developer to debug slow queries.
This commit groups statement statsitics based on its transaction
fingerprintIDs on top of the statement fingerprint ID.

Partially addresses cockroachdb#65106, cockroachdb#59205

Release note (bug fix): statement statsitics are now grouped by
the statement's corresponding transaction fingerprints.
Azhng added a commit to Azhng/cockroach that referenced this issue Nov 18, 2021
fingerprintID

Previously, statement that are executed as part of different
transactions are grouped together resulting in one single
stats entry. This results in weird UX and makes it difficult
for developer to debug slow queries.
This commit groups statement statsitics based on its transaction
fingerprintIDs on top of the statement fingerprint ID.

Partially addresses cockroachdb#65106, cockroachdb#59205

Release note (bug fix): statement statsitics are now grouped by
the statement's corresponding transaction fingerprints.
Azhng added a commit to Azhng/cockroach that referenced this issue Nov 19, 2021
fingerprintID

Previously, statement that are executed as part of different
transactions are grouped together resulting in one single
stats entry. This results in weird UX and makes it difficult
for developer to debug slow queries.
This commit groups statement statsitics based on its transaction
fingerprintIDs on top of the statement fingerprint ID.

Partially addresses cockroachdb#65106, cockroachdb#59205

Release note (bug fix): statement statsitics are now grouped by
the statement's corresponding transaction fingerprints.
Azhng added a commit to Azhng/cockroach that referenced this issue Nov 19, 2021
…d by

transaction fingerprint ID

Previously, the transactionDetail page showed statement statistics aggregated
across **all** executions of that statement fingerprint. This led to
confusing UX and the statement statistics were subsequently removed
from transactionDetail page.
This commit reintroduces statement statistics on the transactionDetail
page. The statistics now are only aggregated across the statement
fingerprints that are part of the selected transaction fingerprint.

Resolves cockroachdb#59205 cockroachdb#65106

Release note (ui change): transactionDetail page now shows statement
statistics scoped by the transaction fingeprint.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-sql-console-general SQL Observability issues on the DB console spanning multiple areas. Includes Cockroach Cloud Console C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants