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

Add human-readable query identifiers in crdb_internal.statement_statistics and DB Console Statements page #91763

Closed
florence-crl opened this issue Nov 11, 2022 · 5 comments · Fixed by #91885
Assignees
Labels
C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception)

Comments

@florence-crl
Copy link

florence-crl commented Nov 11, 2022

Is your feature request related to a problem? Please describe.
In the DB Console SQL Activity->Statements page, there is no unique query identifier in the initial page. When you click into a query, you can see an identifier in the URL, such as http://127.0.0.1:26258/#/statement/true/4705782015019656142?appNames=

In this case 4705782015019656142 is the identifier. When querying crdb_internal.statement_statistics at the main table level there is no identifier that corresponds to 4705782015019656142. The only way to search for a query is using the metadata ->> ‘query’ clause like this:

where metadata ->> 'query' like '%SELECT city, id FROM vehicles WHERE city = $1%'

Sometimes if the full query text as shown in the console is entered, the filter will not find the given query string. You have to use some LIKE condition to find the query string. There are columns fingerprint_id and transaction_fingerprint_id in crdb_internal.statement_statistics but they are BYTE values that are not human-readable.

Describe the solution you'd like
A clear and concise description of what you want to happen.

Describe alternatives you've considered
A clear and concise description of any alternative solutions or features you've considered.

Additional context
Add any other context or screenshots about the feature request here.

cc: @kevin-v-ngo

Jira issue: CRDB-21417

@florence-crl florence-crl added C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) T-sql-observability labels Nov 11, 2022
@maryliag
Copy link
Contributor

This is already possible today. We display the fingerprint id on the overview statement page for that purpose, it's one of the columns (hidden by default). You can select to show the column here:
Screen Shot 2022-11-11 at 5 27 59 PM

then copy the value from the column:
Screen Shot 2022-11-11 at 5 28 08 PM

then on cli you can look for that fingerprint adding the \x at the start of the value like this:
select * from crdb_internal.statement_statistics where fingerprint_id='\xe56448b12fbe7990';

or encoding the fingerprint column like this:
select * from crdb_internal.statement_statistics where encode(fingerprint_id, 'hex')='e56448b12fbe7990';

Does that work for you @florence-crl ?

@florence-crl
Copy link
Author

Thanks @maryliag for the above info and screenshots!

It is not obvious that there are additional column options in that multi-select pop-up. Only because you gave the screenshot above, did I know to scroll down and look for the Statement Fingerprint ID

Initial view of pop-up where scroll bar is not visible. Is there a way to make the scroll bar visible?
Screen Shot 2022-11-11 at 5 42 52 PM

After scrolling down to show the additional column options:
Screen Shot 2022-11-11 at 5 45 43 PM

Also the docs on the Statements table does not mention the hidden columns, so I did not think of looking for it. I will open a docs issue.

@maryliag
Copy link
Contributor

Unfortunately, the visibility of a scrollbar is not something we can't control on our side, is defined by browser/user. What I can do is increase that size or stop at a point that is mid one of the labels so it indicates that there is more to it.

For the docs, looks like something got missed, because there was an issue for the update on the docs, but shows as closed now, but I don't know what happened: cockroachdb/docs#14960
I can see you created an issue for the docs team, and added more context there, so I'll add the PR there for reference.

@florence-crl
Copy link
Author

A user noticed that the Statement Details page has an ID in the URL. In the example URL below, the id is 17122190109225917933

http://127.0.0.1:26258/#/statement/true/17122190109225917933?appNames=

Why is this id different from the Fingerprint ID on the Statements main page?
Also, why is the Fingerprint ID not displayed on the Statement Details page?

@maryliag
Copy link
Contributor

the URL uses the fingerprint id in int64 format and we didn’t show fingerprint on the ui anywhere, so the value didn’t really matter at that point. Now with more people using that value, might be worth changing the url format. That will require a little more work, so can you open a new issue with that request specifically.
I can add the fingerprint in hex format as part of this issue on the details page.

maryliag added a commit to maryliag/cockroach that referenced this issue Nov 14, 2022
Previosuly, it was hard to identify there was more
items on the columns selector, since the scrollbar is confugured
by the user and might not show up right away (it will show once
you hover with mouse and scroll).
This commit changes the height of the filter, making part of
the next options to show up, hinting there is more options
when scrolling.

Part Of cockroachdb#91763

Release note (ui change): Change the height of column selector,
so it can hint there are more options to be selected once
scrolled.
maryliag added a commit to maryliag/cockroach that referenced this issue Nov 15, 2022
Previously, some fingerprint IDs in hex format would have
a leading 0, that was not being returning from the DB calls.
This was causing confusion when seeing the value on the UI
and trying to find the same fringerprint on our tables.

This commits checks the hex values used and add the missing
leading zeros back.

Part Of: cockroachdb#91763

Release note (bug fix): Add leading zeros to fingerprint IDs
with less than 16 characters.
maryliag added a commit to maryliag/cockroach that referenced this issue Nov 15, 2022
Previously, there was no easy way to get a statement or
fingerprint id from their respective details pages,
allowing then the value to be found on our internal tables
using cli.
This commit adds the fingerprint id on the response of
the statement details page.
It also adds the fingeprint ID to the Statement Details
page and Transaction Details page.

Fixes cockroachdb#91763

Release note (ui change): Add fingerprint ID in hex format
to the Statement Details page and Transaction Details page.
craig bot pushed a commit that referenced this issue Nov 15, 2022
91704: *: Improve constraints retrieval in table descriptor r=Xiang-Gu a=Xiang-Gu

This PR tries to improve how we retrieve constraints in a table descriptor. Previously,
it was mostly legacy code carries over from a while ago and nothing hasn't really
changed.
The main change is to introduce `catalog.Constraint` interface, similar to `catalog.Index`
and `catalog.Column`, as the preferred interface for constraint in this layer. Previously,
we would directly expose the underlying protobuf descriptor.

Commit 1 (easy): Rename `catalog.ConstraintToUpdate` to `catalog.Constraint`. 
It's good that we already have an interface that is suitable to be used for our effort.

Commit 2 (easy): Added methods in just renamed `catalog.Constraint` interface for
index-backed-constraints (i.e. PRIMARY KEY or UNIQUE);

Commit 3 (easy): Let `tabledesc.index` struct implement `catalog.Constraint` interface
as we will use it for index-backed-constraints.

Commit 4 (easy): Add a method in `catalog.Constraint` that gets validity of the constraint.

Commit 5 (moderate): Add logic (`ConstraintCache`) to pre-compute all constraints in a
table, categorized by type and validity, so that we can readily retrieve them later. This is the same
idea/technique used for managing index and columns (in `IndexCache` and `ColumnCache`).

Commit 6 (easy): Introduce the new, preferred methods in `catalog.TableDescriptor` to
retrieve constraints from a table.

Commit 7 (easy): Refactor signature of the existing `FindConstraintWithID` method to use
the newly added interface and retrieval methods.

Informs: #90840 
(this PR can unblock #90840)

Release note: None

91867: ui: change height of column selector r=maryliag a=maryliag

Previosuly, it was hard to identify there was more items on the columns selector, since the scrollbar is confugured by the user and might not show up right away (it will show once you hover with mouse and scroll).
This commit changes the height of the filter, making part of the next options to show up, hinting there is more options when scrolling.

Part Of #91763

Before
<img width="322" alt="Screen Shot 2022-11-14 at 2 59 39 PM" src="https://user-images.githubusercontent.com/1017486/201755400-1276e45b-62b8-44c0-a7ff-c337090ad94a.png">

After
<img width="308" alt="Screen Shot 2022-11-14 at 3 02 47 PM" src="https://user-images.githubusercontent.com/1017486/201755427-906e1c3b-e9fa-443b-9508-b2957b38d90b.png">


Release note (ui change): Change the height of column selector, so it can hint there are more options to be selected once scrolled.

Co-authored-by: Xiang Gu <[email protected]>
Co-authored-by: maryliag <[email protected]>
maryliag added a commit to maryliag/cockroach that referenced this issue Nov 15, 2022
Previosuly, it was hard to identify there was more
items on the columns selector, since the scrollbar is confugured
by the user and might not show up right away (it will show once
you hover with mouse and scroll).
This commit changes the height of the filter, making part of
the next options to show up, hinting there is more options
when scrolling.

Part Of cockroachdb#91763

Release note (ui change): Change the height of column selector,
so it can hint there are more options to be selected once
scrolled.
maryliag added a commit that referenced this issue Nov 15, 2022
Previosuly, it was hard to identify there was more
items on the columns selector, since the scrollbar is confugured
by the user and might not show up right away (it will show once
you hover with mouse and scroll).
This commit changes the height of the filter, making part of
the next options to show up, hinting there is more options
when scrolling.

Part Of #91763

Release note (ui change): Change the height of column selector,
so it can hint there are more options to be selected once
scrolled.
craig bot pushed a commit that referenced this issue Nov 15, 2022
90451: Roachtest SSH Retries r=smg260 a=smg260

roachtest:  This change introduces a default retry when encountering an error of `255` (SSH). It is transparent to callers. If desired, it would be relatively simple to expose this via the `cluster` interface and allow callers to specify a retry handler/options on a per command basis, perhaps something like:

``` 
// retry if we get an exit code of 12
retryOpts { 
	opt: retry.Options { .. } , 
	shouldRetryFn: func(res RunResultDetails) bool { return res.RemoteExitStatus == 12 }
}
c.RunE(ctx, nodes, retryOpts, "my_cmd")
```
but this PR does not introduce that.


Today there appears the concept of a "roachprod" and a "command error", the former of which denotes an issue in roachprod handling code, the latter representing an error executing a command over SSH. This PR preserves this behaviour and introduces an updated function signature from `f(i int) ([]byte, error)` to `f(i int) RunResultDetails, error`. `RunResultDetails` has been expanded to capture information about the execution of a command, such as `stderr/our, exit code, error, attempt number`. Any roachprod error will result in an `error` being returned, and set in `RunResultDetails.Err`. Any command error would be only set in `RunResultDetails.Err` with the returned error being nil. This allows callers to differentiate between a roachprod and a command error - something which existing code depends on.

Retry handling code can use a function's returned `RunResultDetails` to determine whether a retry should take place. This PR has default retry handling on `RunResultDetails.RemoteExitStatus == 255`.

Notable changes
- Retry on `255` exit code for any function executed via `ParallelE`
- Consolidated `Run/RunWithDetails` (significant duplication of code)
- Classify exit error at the session.go level so that it is available for all commands and to all callers
- Remove `"echo $?"` option to print exit code at end of cmd. It is not clear why this was done. (Perhaps for visibility in logs?) Accompanying function to parse out the status has also been removed. The exit code is still accessible via the `RunResultDetails`
- Update roachtests to check for SSH marker error instead of checking for `!install.NonZeroExitCode`

Resolves: #73542
Release note: None
Epic: [CRDB-21386](https://cockroachlabs.atlassian.net/browse/CRDB-21386)

91765: utilccl: fix a comment r=andreimatei a=andreimatei

It was missing a key subject.

Release note: None
Epic: None

91884: changfeedccl: deflake TestAlterChangefeedTelemetry  r=stevendanna a=stevendanna

The job system clears the lease asyncronously after cancelation. This
lease clearing transaction can cause a restart in the alter changefeed
transaction, which will lead to different feature counter
counts. Thus, we want to wait for the lease clear. However, the lease
clear isn't guaranteed to happen, so we only wait a few seconds for
it.

Epic: None

Release note: None

91885: roachpb, server, ui: add fingerprint ID to details pages r=maryliag a=maryliag

**Commit 1**
ui: use format from cluster-ui
The format.js file was duplicated on db console,
making it hard to maintain.
This commit replaces all usages of the format
functions on db-console package, to the ones
from cluster-ui.

Epic: None

Release note: None

**Commit 2**
ui: add leading zeros to hex value with less than 16 chars
Previously, some fingerprint IDs in hex format would have
a leading 0, that was not being returning from the DB calls.
This was causing confusion when seeing the value on the UI
and trying to find the same fingerprint on our tables.

This commits checks the hex values used and add the missing
leading zeros back.

Part Of: #91763

Release note (bug fix): Add leading zeros to fingerprint IDs
with less than 16 characters.

**Commit 3**
roachpb, server, ui: add fingerprint ID to details pages
Previously, there was no easy way to get a statement or
fingerprint id from their respective details pages,
allowing then the value to be found on our internal tables
using cli.
This commit adds the fingerprint id on the response of
the statement details page.
It also adds the fingerprint ID to the Statement Details
page and Transaction Details page.

Fixes #91763

<img width="758" alt="Screen Shot 2022-11-14 at 6 53 40 PM" src="https://user-images.githubusercontent.com/1017486/201797428-a04fcbce-e36e-426b-aeb9-bb9250adc4d6.png">
<img width="1524" alt="Screen Shot 2022-11-14 at 6 53 58 PM" src="https://user-images.githubusercontent.com/1017486/201797451-8c7035da-d2bb-4261-ad79-88618f03e7c5.png">




Release note (ui change): Add fingerprint ID in hex format
to the Statement Details page and Transaction Details page.

Co-authored-by: Miral Gadani <[email protected]>
Co-authored-by: Andrei Matei <[email protected]>
Co-authored-by: Steven Danna <[email protected]>
Co-authored-by: maryliag <[email protected]>
@craig craig bot closed this as completed in 66730b8 Nov 15, 2022
maryliag added a commit to maryliag/cockroach that referenced this issue Nov 15, 2022
Previously, some fingerprint IDs in hex format would have
a leading 0, that was not being returning from the DB calls.
This was causing confusion when seeing the value on the UI
and trying to find the same fringerprint on our tables.

This commits checks the hex values used and add the missing
leading zeros back.

Part Of: cockroachdb#91763

Release note (bug fix): Add leading zeros to fingerprint IDs
with less than 16 characters.
maryliag added a commit to maryliag/cockroach that referenced this issue Nov 15, 2022
Previously, there was no easy way to get a statement or
fingerprint id from their respective details pages,
allowing then the value to be found on our internal tables
using cli.
This commit adds the fingerprint id on the response of
the statement details page.
It also adds the fingeprint ID to the Statement Details
page and Transaction Details page.

Fixes cockroachdb#91763

Release note (ui change): Add fingerprint ID in hex format
to the Statement Details page and Transaction Details page.
maryliag added a commit to maryliag/cockroach that referenced this issue Nov 15, 2022
Previously, there was no easy way to get a statement or
fingerprint id from their respective details pages,
allowing then the value to be found on our internal tables
using cli.
This commit adds the fingerprint id on the response of
the statement details page.
It also adds the fingeprint ID to the Statement Details
page and Transaction Details page.

Fixes cockroachdb#91763

Release note (ui change): Add fingerprint ID in hex format
to the Statement Details page and Transaction Details page.
maryliag added a commit that referenced this issue Nov 16, 2022
Previously, some fingerprint IDs in hex format would have
a leading 0, that was not being returning from the DB calls.
This was causing confusion when seeing the value on the UI
and trying to find the same fringerprint on our tables.

This commits checks the hex values used and add the missing
leading zeros back.

Part Of: #91763

Release note (bug fix): Add leading zeros to fingerprint IDs
with less than 16 characters.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants