ui: encode time window in url for metrics charts #101258
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This commit makes URLs to metrics charts more shareable by encoding the time window into the URL. loom: https://www.loom.com/share/d00c40649bc543f18faf981f80b56e64 Known issues: 1. Navigating to a different metrics category via the dropdown will clear the time window params from the URL. No graph functionality is impaired. This is just an inconvenience for someone trying to share a link. The status quo is that nothing is shareable, so this is fine. 2. The logic to decide if the time window should grow as new data becomes available is based on a heuristic. Therefore, it's possible that a user opens a URL with time window params and expects the window to keep growing but we treat it as a "frozen-in-time" custom selection, or vice-versa. Resolves cockroachdb#95971 Epic: none Release note (ui change): The time window selection for metrics charts is now encoded in the URL via query params.
|
This change is |
zachlite
added
backport-23.1.x
maryliag
approved these changes
Apr 11, 2023
There was a problem hiding this comment.
to confirm, this is affecting the time scale also on the SQL Activity page, correct? I mean, if you open the SQL Activity page after using the link with the dates, that same date is being displayed on the SQL Activity page?
Either way
Reviewed 4 of 4 files at r1, all commit messages.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @zachlite)
|
@maryliag, no, this doesn't effect any of the SQL observability pages. |
|
bors r+ |
|
Build succeeded: |
|
Encountered an error creating backports. Some common things that can go wrong:
You might need to create your backport manually using the backport tool. error creating merge commit from 1eb1155 to blathers/backport-release-22.2-101258: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict [] you may need to manually resolve merge conflicts with the backport tool. Backport to branch 22.2.x failed. See errors above. 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
zachlite
added a commit
to zachlite/cockroach
that referenced
this pull request
Jul 7, 2023
Since cockroachdb#101258, the TimeScaleDropdownWithSearchParams can cause infinite re-renders. The exact cause of the bug is not yet diagnosed, but it occurs on the key visualizer page, and seems to be related to the custom duration options. This is tracked in cockroachdb#106395. This commit removes the dependency on TimeScaleDropdownWithSearchParams from the key visualizer, and replaces it with the vanilla TimeScaleDropdown. The custom duration options are still present. Informs: cockroachdb#106395 Epic: none Release note: None
craig bot
pushed a commit
that referenced
this pull request
Jul 7, 2023
103920: opt: loosen restriction on UDF mutations to the same table r=mgartner a=mgartner #### opt: loosen restriction on UDF mutations to the same table To prevent index corruption described in #70731, optbuilder raises an error when a statement performs multiple mutations to the same table. This commit loosens this restriction for UDFs that perform mutations because it is overly strict. --- The index corruption described in #70731 occurs when a statement performs multiple writes to the same table. Any reads performed by successive writes see the snapshot of data as of the beginning of the statement. They do not read values as of the most recent write within the same statement. Because these successive writes are based on stale data, they can write incorrect KVs and cause inconsistencies between primary and secondary indexes. Each statement in a UDF body is essentially a child of the statement that is invoking the UDF. Mutations within UDFs are not as susceptible to the inconsistencies described above because a UDF with a mutation must be VOLATILE, and each statement in a VOLATILE UDFs reads at the latest sequence number. In other words, statements within UDFs can see previous writes made by any outer statement. This prevents inconsistencies due to writes based on stale reads. Therefore, the restriction that prevents multiple writes to the same table can be lifted in some cases when the writes are performed in UDFs. However, we cannot forgo restrictions for all writes in UDFs. A parent statement that calls a UDF cannot be allowed to mutate the same table that the UDF did. Unlike subsequent statements in the UDF after the write, the parent statement will not see the UDF's writes, and inconsistencies could occur. To define acceptable mutations to the same table within UDFs, we define a statement tree that represents the hierarchy of statements and sub-statements in a query. A sub-statement `sub` is any statement within a UDF. `sub`'s parent is the statement invoking the UDF. Other statements in the same UDF as `sub` are the `sub`'s siblings. Any statements in a UDF invoked by `sub` are `sub`'s children. For example, consider: CREATE FUNCTION f1() RETURNS INT LANGUAGE SQL AS 'SELECT 1'; CREATE FUNCTION f2() RETURNS INT LANGUAGE SQL AS 'SELECT 2 + f3()'; CREATE FUNCTION f3() RETURNS INT LANGUAGE SQL AS 'SELECT 3'; SELECT f1(), f2(), f3(); The statement tree for this SELECT would be: root: SELECT f1(), f2(), f3() ├── f1: SELECT 1 ├── f2: SELECT 2 + f3() │ └── f3: SELECT 3 └── f3: SELECT 3 We define multiple mutations to the same table as safe if, for every possible path from the root statement to a leaf statement, either of the following is true: 1. There is no more than one mutation to any table. 2. Or, any table with multiple mutations is modified only by simple INSERTs without ON CONFLICT clauses. As a consequence of this definition, a UDF is now allowed to mutate the same table as long as it does so in different statements in its body. Such statements are siblings in the statement tree, and therefore do not share any path from root to leaf. For example, this is now allowed: CREATE FUNCTION ups(a1 INT, a2 INT) RETURNS VOID LANGUAGE SQL AS $$ UPSERT INTO a VALUES (a1); UPSERT INTO a VALUES (a2); $$ Similarly, successive invocations of the same UDF that mutates a table are now allowed: CREATE FUNCTION upd(k0 INT, v0 INT) RETURNS VOID LANGUAGE SQL AS $$ UPDATE kv SET v = v0 WHERE k = k0; $$; SELECT upd(1, 2), upd(1, 3); The `statementTree` data structure has been added to enforce this definition. See its documentation for more details. Note: These restrictions will likely need to be revisited once we support recursive UDFs. Epic: CRDB-25388 Informs #70731 Release note: None 106354: sql: report contention on writes in EXPLAIN ANALYZE r=yuzefovich a=yuzefovich This commit adds the contention events listener to `planNodeToRowSource` which allows us to add contention time information for mutation planNodes to be shown in EXPLAIN ANALYZE. Fixes: #106266. Release note (sql change): CockroachDB now reports contention time encountered while executing mutation statements (INSERT, UPSERT, UPDATE, DELETE) when run via EXPLAIN ANALYZE. 106398: ui: fix infinite re-render on the key visualizer page r=zachlite a=zachlite Since #101258, the TimeScaleDropdownWithSearchParams can cause infinite re-renders. The exact cause of the bug is not yet diagnosed, but it occurs on the key visualizer page, and seems to be related to the custom duration options. This is tracked in #106395. This commit removes the dependency on TimeScaleDropdownWithSearchParams from the key visualizer, and replaces it with the vanilla TimeScaleDropdown. The custom duration options are still present. Informs: #106395 Epic: none Release note: None 106409: workload: ignore QueryCanceled in random schema change test r=rafiss a=rafiss fixes #105299 Release note: None Co-authored-by: Marcus Gartner <[email protected]> Co-authored-by: Yahor Yuzefovich <[email protected]> Co-authored-by: zachlite <[email protected]> Co-authored-by: Rafi Shamim <[email protected]>
blathers-crl bot
pushed a commit
that referenced
this pull request
Jul 7, 2023
Since #101258, the TimeScaleDropdownWithSearchParams can cause infinite re-renders. The exact cause of the bug is not yet diagnosed, but it occurs on the key visualizer page, and seems to be related to the custom duration options. This is tracked in #106395. This commit removes the dependency on TimeScaleDropdownWithSearchParams from the key visualizer, and replaces it with the vanilla TimeScaleDropdown. The custom duration options are still present. Informs: #106395 Epic: none Release note: None
maryliag
pushed a commit
that referenced
this pull request
Jul 24, 2023
Since #101258, the TimeScaleDropdownWithSearchParams can cause infinite re-renders. The exact cause of the bug is not yet diagnosed, but it occurs on the key visualizer page, and seems to be related to the custom duration options. This is tracked in #106395. This commit removes the dependency on TimeScaleDropdownWithSearchParams from the key visualizer, and replaces it with the vanilla TimeScaleDropdown. The custom duration options are still present. Informs: #106395 Epic: none Release note: None
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This commit makes URLs to metrics charts more shareable by encoding the time window into the URL.
loom: https://www.loom.com/share/d00c40649bc543f18faf981f80b56e64
Known issues:
Navigating to a different metrics category via the dropdown will clear the time window params from the URL. No graph functionality is impaired. This is just an inconvenience for someone trying to share a link. The status quo is that nothing is shareable, so this is fine.
The logic to decide if the time window should grow as new data becomes available is based on a heuristic. Therefore, it's possible that a user opens a URL with time window params and expects the window to keep growing but we treat it as a "frozen-in-time" custom selection, or vice-versa.
Resolves #95971
Epic: none
Release note (ui change): The time window selection for metrics charts is now encoded in the URL via query params.