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.

Sign up for GitHub

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

Jump to bottom

ui: re-enable displaying internal statement in SQL Activity Page #79547

Closed
Azhng opened this issue Apr 6, 2022 · 5 comments · Fixed by #86679
Closed

ui: re-enable displaying internal statement in SQL Activity Page #79547

Azhng opened this issue Apr 6, 2022 · 5 comments · Fixed by #86679
Assignees
@maryliag
Labels
A-sql-observability Related to observability of the SQL layer C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior.

Comments

@Azhng
Copy link
Contributor

Azhng commented Apr 6, 2022 •
edited by cockroach-jira-scripts
Loading

Previously, we stop sending internal statements to SQL Activity Page due to long load time of the page. However, this becomes a problem when we need to debug CRDB internal statements more difficult. Often we need to request statement bundle of CRDB internal statements to understand the issue (e.g. Jobs system, SQL Stats cleanup job). Without displaying internal statements in the Statement Page, we have no way of requesting statement bundles of those internal statements.

cc: @ajwerner

Jira issue: CRDB-14901
@Azhng Azhng added C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. A-sql-observability Related to observability of the SQL layer T-sql-observability labels Apr 6, 2022
@Azhng Azhng mentioned this issue Apr 8, 2022
Merged
Azhng added a commit to Azhng/cockroach that referenced this issue Apr 11, 2022
@Azhng
sql: add builtin to request statement bundles
01b4168 
Makes cockroachdb#79547 less painful.

Previously, there was no way to request statement bundle builtins for
internal statements.
This commit introduces crdb_internal.request_statement_bundle builtin
which allows statement bundle being requested from SQL CLI. The new
builtin takes three parameters:
* statement fingerprint text
* minimum execution latency for the statement
* length of duration the statement bundle request will stay valid for
VIEWACTIVITY or VIEWACTIVITYREDACTED permissions are required to use
this builtin.

Release note (sql change): new crdb_internal.request_statement_bundle builtin
allows statement bundle being requested from SQL CLI. The new builtin takes
three parameters:
* statement fingerprint text
* minimum execution latency for the statement
* length of duration the statement bundle request will stay valid for.
VIEWACTIVITY or VIEWACTIVITYREDACTED permissions are required to use
this builtin.
@otan
Copy link
Contributor

otan commented Apr 13, 2022

See #79916 for an example of the priority of this - it's somewhat surprising we don't show internal queries which may be taking up a lot of time there.

@rafiss rafiss mentioned this issue Apr 13, 2022
Closed
Azhng added a commit to Azhng/cockroach that referenced this issue May 6, 2022
@Azhng
sql: add builtin to request statement bundles
d5c5152 
Makes cockroachdb#79547 less painful.

Previously, there was no way to request statement bundle builtins for
internal statements.
This commit introduces crdb_internal.request_statement_bundle builtin
which allows statement bundle being requested from SQL CLI. The new
builtin takes three parameters:
* statement fingerprint text
* minimum execution latency for the statement
* length of duration the statement bundle request will stay valid for
VIEWACTIVITY role option is required to use this builtin. If user has
VIEWACTIVITYREDACTED role option, user is not allowed to use this builtin.

Release note (sql change): new crdb_internal.request_statement_bundle builtin
allows statement bundle being requested from SQL CLI. The new builtin takes
three parameters:
* statement fingerprint text
* minimum execution latency for the statement
* length of duration the statement bundle request will stay valid for.
VIEWACTIVITY role option is required to use this builtin. If user has
VIEWACTIVITYREDACTED role option, user is not allowed to use this builtin.
Azhng added a commit to Azhng/cockroach that referenced this issue May 6, 2022
@Azhng
sql: add builtin to request statement bundles
f248c50 
Makes cockroachdb#79547 less painful.

Previously, there was no way to request statement bundle builtins for
internal statements.
This commit introduces crdb_internal.request_statement_bundle builtin
which allows statement bundle being requested from SQL CLI. The new
builtin takes three parameters:
* statement fingerprint text
* minimum execution latency for the statement
* length of duration the statement bundle request will stay valid for
VIEWACTIVITY or ADMIN role option is required to use this builtin. If user has
VIEWACTIVITYREDACTED role option, user is not allowed to use this builtin.

Release note (sql change): new crdb_internal.request_statement_bundle builtin
allows statement bundle being requested from SQL CLI. The new builtin takes
three parameters:
* statement fingerprint text
* minimum execution latency for the statement
* length of duration the statement bundle request will stay valid for.
VIEWACTIVITY or ADMIN role option is required to use this builtin. If user has
VIEWACTIVITYREDACTED role option, user is not allowed to use this builtin.
Azhng added a commit to Azhng/cockroach that referenced this issue May 6, 2022
@Azhng
sql: add builtin to request statement bundles
ce606f5 
Makes cockroachdb#79547 less painful.

Previously, there was no way to request statement bundle builtins for
internal statements.
This commit introduces crdb_internal.request_statement_bundle builtin
which allows statement bundle being requested from SQL CLI. The new
builtin takes three parameters:
* statement fingerprint text
* minimum execution latency for the statement
* length of duration the statement bundle request will stay valid for
VIEWACTIVITY or ADMIN role option is required to use this builtin. If user has
VIEWACTIVITYREDACTED role option, user is not allowed to use this builtin.

Release note (sql change): new crdb_internal.request_statement_bundle builtin
allows statement bundle being requested from SQL CLI. The new builtin takes
three parameters:
* statement fingerprint text
* minimum execution latency for the statement
* length of duration the statement bundle request will stay valid for.
VIEWACTIVITY or ADMIN role option is required to use this builtin. If user has
VIEWACTIVITYREDACTED role option, user is not allowed to use this builtin.
craig bot pushed a commit that referenced this issue May 6, 2022
@Azhng @michae2
Merge #79693 #81102
e04373b 
79693: sql: add builtin to request statement bundles r=Azhng a=Azhng

Makes #79547 less painful.

Previously, there was no way to request statement bundle builtins for
internal statements.
This commit introduces crdb_internal.request_statement_bundle builtin
which allows statement bundle being requested from SQL CLI. The new
builtin takes three parameters:
* statement fingerprint text
* minimum execution latency for the statement
* length of duration the statement bundle request will stay valid for
VIEWACTIVITY or ADMIN role option is required to use this builtin. If user has
VIEWACTIVITYREDACTED role option, user is not allowed to use this builtin.

Release note (sql change): new crdb_internal.request_statement_bundle builtin
allows statement bundle being requested from SQL CLI. The new builtin takes
three parameters:
* statement fingerprint text
* minimum execution latency for the statement
* length of duration the statement bundle request will stay valid for.
VIEWACTIVITY or ADMIN role option is required to use this builtin. If user has
VIEWACTIVITYREDACTED role option, user is not allowed to use this builtin.

81102: roachtest: fix costfuzz log on error while running perturbed statement r=rytaft a=michae2

Also log the unperturbed statement when the costfuzz test encounters an
error executing the perturbed statement.

This can be fixed by hand, but it's easier if the costfuzz log is
correct.

Informs: #81032

Release note: None

Co-authored-by: Azhng <[email protected]>
Co-authored-by: Michael Erickson <[email protected]>
@rail
Copy link
Member

rail commented May 25, 2022

Manually synced with Jira

@vy-ton vy-ton mentioned this issue May 26, 2022
Closed
1 task
@jlinder jlinder removed the sync-me-3 label May 27, 2022
@rafiss
Copy link
Collaborator

rafiss commented Aug 3, 2022

Copying over comment from @vy-ton :

The perf issues were due to user-generated statements so I wonder if there is a path to just reenable internal statements. Users at the upper limit were still going to have issues but we could improve the UX of users at the lower end. Maybe we expose a setting to disable internal but still have the default to enable.

@vy-ton
Copy link
Contributor

vy-ton commented Aug 4, 2022

Thanks Rafi, @kevin-v-ngo thoughts on having a setting here?

@rafiss
Copy link
Collaborator

rafiss commented Aug 16, 2022

https://github.com/cockroachlabs/support/issues/1756 is a support case where we could really use more insight into internal statement activity.

I think if we can make the "hide internal queries" behavior opt-in with a setting (and ideally backport that to v22.1), it would be very beneficial. (ccing @maryliag as well)

@ZhouXing19 ZhouXing19 mentioned this issue Aug 17, 2022
Merged
@maryliag maryliag self-assigned this Aug 23, 2022
@maryliag maryliag mentioned this issue Aug 23, 2022
Merged
craig bot pushed a commit that referenced this issue Aug 25, 2022
@aliher1911 @tbg
@maryliag @erikgrinaker @msbutler
Merge #86608 #86645 #86679 #86748 #86809
54bc65f 
86608: batcheval: add latch key protecting range key stats update r=erikgrinaker a=aliher1911

Previously GC needed to get a read latch with max timestamp to
ensure that range tombstones are not modified during GC. This
is causing all writers to get stuck in queue while GC is validating
request and removing range tombstone.
This commit adds a dedicated latch key
LocalRangeRangeTombstoneStatsUpdateLockSuffix to address the problem.
All range tombstone writers obtain this read latch on top of the write
latches for the ranges they are interested to update.
GC on the other hand will obtain write latch on that key. This
approach allows point writers to proceed during GC, but will block new
range tombstones from being written. Non conflicting writes of range
tombstones could still proceed since their write latch ranges don't
overlap.

Release justification: this is a safe change as range tombstone
behaviour is not enabled yet and the change is needed to address
potential performance regressions.

Release note: None

86645: kvserver: log when raft send/recv queue fills up r=pavelkalinnikov a=tbg

Inspired by https://github.com/cockroachlabs/support/issues/1770.

If either the raft send or receive queue fills up, wide-spread outages
can occur as replication progress stalls. We have metrics that can
indicate this, but straightforward logging is also appropriate to direct
attention to the fact, which this commit achieves.

Touches #79755

Release justification: important logging improvement
Release note: None


86679: server,ui: show internal stats with new cluster setting r=maryliag a=maryliag

Previously, we were not showing internal results on
fingerprint options on SQL Activity.
A new cluster setting created `sql.stats.response.show_internal`
can be set to `true` and internal statistics will be
displayed on SQL Activity page.

Fixes #79547

https://www.loom.com/share/1b89ba99a7c247edadb5c8b0d127755c

Release justification: low risk, high benefit change
Release note (sql change): New cluster setting
`sql.stats.response.show_internal` with default value of `false`
can be set to true, to display information about internal stats
on SQL Activity page, with fingerprint option.

86748: storage: rename `MVCCRangeKeyStack.FirstAbove/Below` r=tbg a=erikgrinaker

This patch renames `FirstAbove/Below` to `FirstAtOrAbove/Below`, for
clarity.

Release justification: bug fixes and low-risk updates to new functionality

Release note: None

86809: backupccl: set kv.bulkio.write_metadata_sst.enabled to default false r=stevendanna a=msbutler

This patch sets write_metadata_sst cluster setting to false in prep for the
22.2 branch cut, as there's additional worked required before this feature gets
used in production.

Setting this to false will also stop the roachtest in #86289 from consistently
failing due to #86806.

Fixes #86289

Release note: none

Release justification: prevents using an experimental feature by default

Co-authored-by: Oleg Afanasyev <[email protected]>
Co-authored-by: Tobias Grieger <[email protected]>
Co-authored-by: Marylia Gutierrez <[email protected]>
Co-authored-by: Erik Grinaker <[email protected]>
Co-authored-by: Michael Butler <[email protected]>
@craig craig bot closed this as completed in 3750821 Aug 25, 2022
maryliag added a commit to maryliag/cockroach that referenced this issue Aug 25, 2022
@maryliag
server,ui: show internal stats with new cluster setting
a05ccc2 
Previously, we were not showing internal results on
fingerprint options on SQL Activity.
A new cluster setting created `sql.stats.response.show_internal`
can be set to `true` and internal statistics will be
displayed on SQL Activity page.

Fixes cockroachdb#79547

Release justification: low risk, high benefit change
Release note (sql change): New cluster setting
`sql.stats.response.show_internal` with default value of `false`
can be set to true, to display information about internal stats
on SQL Activity page, with fingerprint option.
@maryliag maryliag mentioned this issue Aug 25, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@maryliag maryliag

Labels
A-sql-observability Related to observability of the SQL layer C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

server,ui: show internal stats with new cluster setting maryliag/cockroach
7 participants
@rail @jlinder @maryliag @rafiss @otan @Azhng @vy-ton