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

Remove search by sampled plan behavior on the statements overview page #83155

Closed
kevin-v-ngo opened this issue Jun 21, 2022 · 1 comment · Fixed by #83423
Closed

Remove search by sampled plan behavior on the statements overview page #83155

kevin-v-ngo opened this issue Jun 21, 2022 · 1 comment · Fixed by #83423
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 Jun 21, 2022

Remove search by sampled plan behavior on the statements overview page. We do not search through all plans captured per statement fingerprint and this old behavior based on the sampled plan is confusing and not always accurate.

related issue: #71615

Jira issue: CRDB-16890

@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. A-sql-console-general SQL Observability issues on the DB console spanning multiple areas. Includes Cockroach Cloud Console labels Jun 21, 2022
@xinhaoz xinhaoz self-assigned this Jun 27, 2022
craig bot pushed a commit that referenced this issue Jun 27, 2022
81079: tracing: aggregate OperationMetadata on span Finish() r=andreimatei a=adityamaru

This change adds a `ChildrenMetadata` map to `crdbspan` that is a
mapping from operation to the operations' aggregated metadata. This
map is updated whenever a child of the `crdbSpan` finishes, with metadata
from all spans in the finishing childs' Recording. The map is therefore
a bucketed view of all the operations being traced by a span.

The motivation for this change is to surface more metadata about the
suboperations being traced in a spans' Recording. This could in turn provide
more o11y into why a job is slow/stuck, or where the performance of a
distributed operation is bottlenecked.

As part of a span Finish()ing, the span fetches its Recording with the spans'
configured verbosity. Prior to this change the recording would then be
processed as follows:

*Verbose Recording*

In the case of Verbose recording the spans in the recording are added to the parents'
`finishedChildren` slice provided we have not exceeded the maximum number of
children a parent can track.

*Structured Recording*

In the case of a Structured recording, only the StructuredEvents from the spans in
the recording are copied into the parent.

With this change, in both the Verbose and Structured recording mode, a finishing span
is also responsible for rolling up the OperationMetadata of all the spans in its
recording. This involves updating the parents' `childrenMetadata` mapping with:

1) an entry for the finishing span.
2) an entry for each of the finishing spans' Finish()ed children.
3) an entry for each of the finishing spans' open children, and their children recursively

The logic for 2) and 3) is subsumed in the method responsible for getting
the finishing spans' recording. Notably, GetRecording(...) for both Structured and Verbose
recordings, populate the root of the recording with OperationMetadata of all
finished and open children in the recording.

As an example when we are done finishing `child`:

```
parent
  child (finished_C: 4s, finished_D: 3s)
    open_A (finished_B: 1s)
      finished_B
    finished_C (finished_D: 3s)
      finished_D
```

We'd expect `parent` to have:
`{child: 10s, finished_C: 4s, finished_D: 3s, open_A: 3s, finished_B: 1s}`

Given that Finish()ing a child, and importing a remote recording into a span
share the same code path, the above semantics also apply to a remote recording
being imported into a parent span.

Fixes: #80391

Release note: None

82667: storage: add `MVCCTimeInterval` block property for range keys r=jbowens a=erikgrinaker

This patch adds `MVCCTimeInternal` block property collection and
filtering for range keys, which allows using time-bound iterators with
range keys.

Range keys will only be written once the `MVCCRangeTombstones` version
gate is enabled.

Resolves #82596.

Release note: None

83107: ui: make Metrics and SQL timepicker align r=maryliag a=maryliag

Previously, the timepicker from Metrics page and
the timepicker on SQL Activity pages acted independently.
Now, if the value of one changes, the other value changes
to the same period selected.

This commit also fixes a bug where the period selected
would change to a custom value if the Metrics page was
refreshed.

Fixes #78187
Fixes #82152

Release note (ui change): The period selected on the Metrics
page and the SQL Activity pages are now aligned. If the user
changes in one page, the value will be the same for the other.

Release note (bug fix): The period selected on Metrics page 
continues the same when refreshing the page, no longer changing 
to a custom period.

83400: awsdms: further deflake roachtest r=rafiss a=otan

Once the connection is tested, we also have to ensure the status of
the DMS endpoint connection is successful before continuing.
Otherwise DMS may fail to startup.

Resolves #83369

Release note: None

83423: cluster-ui/ui: remove ability to search statements by plan r=xinhaoz a=xinhaoz

Closes  #83155

Previously, we allowed statements in the statements page to
searchable by text in the explain plan. This was before we
returned multiple plans for a statement fingerprint. This commit
removes the explain plan text as part of the searchable string, as
this feature could  now lead to confusing behaviour.

Release note (ui change): In the statements page, users can no
longer filter statements by searching for text in the EXPLAIN
plan.

83427: ui: update labels on Session Details page r=maryliag a=maryliag

Update labels so all of them use the same
format.

Fixes #80350

Release note: None

83457: ccl/sqlproxyccl: fix TestConnectionMigration test flake r=JeffSwenson,rafiss a=jaylim-crl

Fixes #83096.

It appears that database/sql does not provide any thread-safe guarantees for
methods on the Conn type except the Close method. This commit fixes a test-only
issue that causes a panic when there's a race between internal Conn methods
by ensuring that Conn methods are used in a single-threaded way.

Release note: None

Release justification: sqlproxy only test change.

83460: ui: explain eslint plugin prequisite r=laurenbarker a=sjbarag

A recent commit [1] introduced a custom eslint plugin that's hosted in
this repo, but didn't add documentation around building that plugin to
resolve errors reported in IDEs. Explain that eslint-plugin-crdb should
be built to silence errors from eslint that get reported in editors.

[1] ba68179 (ui: use esbuild-loader in webpack configs, 2022-05-26)

Release note: None

Co-authored-by: Aditya Maru <[email protected]>
Co-authored-by: Erik Grinaker <[email protected]>
Co-authored-by: Marylia Gutierrez <[email protected]>
Co-authored-by: Oliver Tan <[email protected]>
Co-authored-by: Xin Hao Zhang <[email protected]>
Co-authored-by: Jay <[email protected]>
Co-authored-by: Sean Barag <[email protected]>
@craig craig bot closed this as completed in 7c57bce Jun 27, 2022
xinhaoz added a commit to xinhaoz/cockroach that referenced this issue Jun 29, 2022
Closes  cockroachdb#83155

Previously, we allowed statements in the statements page to
searchable by text in the explain plan. This was before we
returned multiple plans for a statement fingerprint. This commit
removes the explain plan text as part of the searchable string, as
this feature could  now lead to confusing behaviour.

Release note (ui change): In the statements page, users can no
longer filter statements by searching for text in the EXPLAIN
plan.
xinhaoz added a commit to xinhaoz/cockroach that referenced this issue Jun 30, 2022
Closes  cockroachdb#83155

Previously, we allowed statements in the statements page to
searchable by text in the explain plan. This was before we
returned multiple plans for a statement fingerprint. This commit
removes the explain plan text as part of the searchable string, as
this feature could  now lead to confusing behaviour.

Release note (ui change): In the statements page, users can no
longer filter statements by searching for text in the EXPLAIN
plan.
@kevin-v-ngo
Copy link
Author

reverted: #71615

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.

2 participants