ui/cluster-ui: add 'Interval Start Time' column to stmts/txns tables

[xinhaoz]

Resolves #69648, #70545

This commit adds the Interval Start Time (UTC) column to stmt and txn
tables. Statements and transactions are now both grouped by their aggregated_ts
field in addition to the stmt / fingerprint id. On the statement details page,
the 'Interval Start Time' has also been added to the details panel.

To support viewing of statements grouped by aggregation interval start time,
a new query parameter has been added to statement details pages.
If aggregated_ts is set, it will display the statement details for statements
aggregated at that interval, using data from combined statements API response.
If unset, we will show data aggregated over the current date range.

Release note (ui change): A new column, 'Interval Start Time (UTC)', has
been added to both statement and transaction tables. The column represents
the start time in UTC of the stats aggregation interval for a statement.
By default, the aggregation interval is 1 hour. 'Interval Start Time' has
been added to the statement details page under 'Statement Details'.

A new query parameter has been added to statement details pages.
If the search param aggregated_ts is set, it will display the statement details
for statements aggregated at that interval. If unset, we will display the statement
details for the statement aggregated over the current date range.

---

Statements Table:

Transactions Table:

details page:

[cockroach-teamcity]

This change is 

[matthewtodd]

I like it! Just a couple of questions.

Reviewed 37 of 37 files at r1, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @xinhaoz)

---

pkg/ui/workspaces/cluster-ui/src/statementDetails/statementDetails.fixture.ts, line 121 at r1 (raw file):

// Mock value for aggregated timestamp: Sep 15 2021 01:00:00 GMT+0000
const aggregatedTs = 1631667600;

nit: I don't know if this particular value is important, but you could do something like Date.parse("Sep 15 2021 01:00:00 UTC") / 1000 to make the value self-describing.

---

pkg/ui/workspaces/cluster-ui/src/statsTableUtil/statsTableUtil.tsx, line 95 at r1 (raw file):

// The function parameter is a StatisticType, which represents the type
// of data the statistics are based on (e.g. statements, transactions, or transactionDetails). The
// StatisticType is used to modify the content of the tooltip.

nit: Remove this StatisticType comment since the parameter's gone away?

---

pkg/ui/workspaces/cluster-ui/src/util/convert.ts, line 65 at r1 (raw file):

    return defaultIfNull;
  }
  return timestamp.seconds.toNumber() + NanoToMilli(timestamp.nanos);

Is this correct? It looks like we're adding (integer) seconds with (integer) milliseconds.

---

pkg/ui/workspaces/db-console/src/util/convert.ts, line 76 at r1 (raw file):

    return defaultIfNull;
  }
  return timestamp.seconds.toNumber() + NanoToMilli(timestamp.nanos);

Same.

---

pkg/ui/workspaces/db-console/src/util/query.ts, line 34 at r1 (raw file):

        : null,
    ),
  ).join("&");

nit: Just curious, in the other location you used URLSearchParams. Any reason for the difference? (And I'm also wondering if these are worth sharing, rather than defining twice...)

[maryliag]

Reviewed 24 of 37 files at r1, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @matthewtodd and @xinhaoz)

---

pkg/ui/workspaces/cluster-ui/src/statsTableUtil/statsTableUtil.tsx, line 95 at r1 (raw file):

Previously, matthewtodd (Matthew Todd) wrote…

nit: Remove this StatisticType comment since the parameter's gone away?

we don't have the parameter for the new column, but it is still being used on other titles, so the comment still stands :)

---

pkg/ui/workspaces/cluster-ui/src/transactionsPage/transactionsPage.tsx, line 79 at r1 (raw file):

  filters?: Filters;
  statementFingerprintIds: Long[] | null;
  aggregatedTs: protos.google.protobuf.ITimestamp | null;

nit: add the type a little above like we do with the other types and then call here

type ITimestamp = protos.google.protobuf.ITimestamp
...
aggregatedTs: ITimestamp | null

---

pkg/ui/workspaces/cluster-ui/src/transactionsPage/utils.ts, line 60 at r1 (raw file):

export const getStatementsByFingerprintIdAndTime = (
  statementFingerprintIds: Long[],
  timestamp: protos.google.protobuf.ITimestamp | null,

nit: similar to me other comment

---

pkg/ui/workspaces/cluster-ui/src/transactionsTable/transactionsTable.tsx, line 138 at r1 (raw file):

    },
    {
      name: "interval start time",

this value needs to be the same key you added to the stats util file, so intervalStartTime, otherwise it won't load on the column selector

---

pkg/ui/workspaces/cluster-ui/src/util/appStats/appStats.ts, line 244 at r1 (raw file):

export function statementKey(stmt: ExecutionStatistics): string {
  return (
    stmt.statement + stmt.implicit_txn + stmt.database + stmt.aggregated_ts

nit: update the comment with the new parameters being used

---

pkg/ui/workspaces/db-console/src/util/appStats.ts, line 228 at r1 (raw file):

export function statementKey(stmt: ExecutionStatistics): string {
  return (
    stmt.statement + stmt.implicit_txn + stmt.database + stmt.aggregated_ts

update the comment on the function

---

pkg/ui/workspaces/db-console/src/views/transactions/transactionsPage.tsx, line 35 at r1 (raw file):

  (state: AdminUIState) => state.cachedData.statements,
  (state: CachedDataReducerState<StatementsResponseMessage>) => {
    if (!state.data || !state.valid) return null;

nit: can you add some comment about what is a valid statement?

[Azhng]

Reviewed 11 of 37 files at r1.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @xinhaoz)

---

-- commits, line 6 at r1:
Previously, if the selected time range is more than 1 aggregation interval, we would have 1 entry for each distinct stmt/txn fingerprint, now it seems like if the selected time range is more than 1 aggregation interval, we would have multiple entry for each distinct stmt/txn fingerprint. I'm curious that, with this change, is it still possible for users to view statistics for stmts/txns aggregated across the entire selected time range?

---

pkg/ui/workspaces/cluster-ui/src/statementDetails/statementDetails.tsx, line 172 at r1 (raw file):

  // If there was no aggregated_ts requested in the search parameters, we show
  // the latest statement from in-memory stats. This is mostly relevant for displaying
  // statement details from active queries from the session details page.

I'm a bit confused by the second part of this comment. SQL Stats doesn't have any information on the "Active Queries", since the active queries are being tracked by a different system. So I'm a bit puzzled here on how is statement details related to active queries?

---

pkg/ui/workspaces/cluster-ui/src/statementDetails/statementDetails.tsx, line 365 at r1 (raw file):

  componentDidMount() {
    const req = statementsRequestFromProps(this.props);

nit: perhaps refactor this into a helper func since both componentDidMount and componentWillUnmount reuse the same code.

[xinhaoz]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @Azhng, @maryliag, @matthewtodd, and @xinhaoz)

---

pkg/ui/workspaces/cluster-ui/src/transactionsPage/utils.ts, line 60 at r1 (raw file):

Previously, maryliag (Marylia Gutierrez) wrote…

nit: similar to me other comment

Done.

---

pkg/ui/workspaces/cluster-ui/src/transactionsTable/transactionsTable.tsx, line 138 at r1 (raw file):

Previously, maryliag (Marylia Gutierrez) wrote…

this value needs to be the same key you added to the stats util file, so intervalStartTime, otherwise it won't load on the column selector

Done.

---

pkg/ui/workspaces/cluster-ui/src/util/convert.ts, line 65 at r1 (raw file):

Previously, matthewtodd (Matthew Todd) wrote…

Is this correct? It looks like we're adding (integer) seconds with (integer) milliseconds.

Fixed! Nice catch.

---

pkg/ui/workspaces/cluster-ui/src/util/appStats/appStats.ts, line 244 at r1 (raw file):

Previously, maryliag (Marylia Gutierrez) wrote…

nit: update the comment with the new parameters being used

Done.

---

pkg/ui/workspaces/db-console/src/util/appStats.ts, line 228 at r1 (raw file):

Previously, maryliag (Marylia Gutierrez) wrote…

update the comment on the function

Done.

---

pkg/ui/workspaces/db-console/src/util/convert.ts, line 76 at r1 (raw file):

Previously, matthewtodd (Matthew Todd) wrote…

Same.

Fixed.

---

pkg/ui/workspaces/db-console/src/util/query.ts, line 34 at r1 (raw file):

Previously, matthewtodd (Matthew Todd) wrote…

nit: Just curious, in the other location you used URLSearchParams. Any reason for the difference? (And I'm also wondering if these are worth sharing, rather than defining twice...)

Ah, I just moved this function over from where it was previously defined in api.ts since they seemed to be doing the same thing (but this function ignores nullish params). I've now matched it with the one from cluster-ui, as that one is more up to date.
I don't think cluster-ui currently packages its query utils, hence the duplication.

---

pkg/ui/workspaces/db-console/src/views/transactions/transactionsPage.tsx, line 35 at r1 (raw file):

Previously, maryliag (Marylia Gutierrez) wrote…

nit: can you add some comment about what is a valid statement?

Done, and changed it to inFlight for db-console which I think makes more sense. My intention here was I wanted to show the loading screen while we are fetching new data, for when the user sets a new date range / resetting the date. The cluster-ui states don't have an inFilght state, but !state.isValid should accomplish the same loading behaviour.

[xinhaoz]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @Azhng, @kevin-v-ngo, @maryliag, @matthewtodd, and @xinhaoz)

---

-- commits, line 6 at r1:

Previously, Azhng (Archer Zhang) wrote…

Previously, if the selected time range is more than 1 aggregation interval, we would have 1 entry for each distinct stmt/txn fingerprint, now it seems like if the selected time range is more than 1 aggregation interval, we would have multiple entry for each distinct stmt/txn fingerprint. I'm curious that, with this change, is it still possible for users to view statistics for stmts/txns aggregated across the entire selected time range?

As of this change, it will not be possible to view the stmt/txn fingerprint aggregated over the user-specified time range, and there will be multiple entries for a stmt/txn fingerprint (with the specified agg interval start time). I am going off the figma, but @kevin-v-ngo can probably confirm the expected view/behaviour here.

---

pkg/ui/workspaces/cluster-ui/src/statementDetails/statementDetails.tsx, line 172 at r1 (raw file):

Previously, Azhng (Archer Zhang) wrote…

I'm a bit confused by the second part of this comment. SQL Stats doesn't have any information on the "Active Queries", since the active queries are being tracked by a different system. So I'm a bit puzzled here on how is statement details related to active queries?

Currently on session details, the active query links to the statement details page for the query based on the URL created from the active query info. Are you saying we will never have collected information for the active query?

[maryliag]

Reviewed 3 of 37 files at r1, 13 of 14 files at r2, 1 of 1 files at r3.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @Azhng, @kevin-v-ngo, @matthewtodd, and @xinhaoz)

---

pkg/ui/workspaces/db-console/src/util/appStats.ts, line 228 at r1 (raw file):

Previously, xinhaoz (Xin Hao Zhang) wrote…

Done.

You marked as done, but you forgot to address this :)

[xinhaoz]

---

pkg/ui/workspaces/db-console/src/util/appStats.ts, line 228 at r1 (raw file):

Previously, maryliag (Marylia Gutierrez) wrote…

You marked as done, but you forgot to address this :)

Whoops, didn't hit save on the file 🤦 Done!

[maryliag]

can you add the issue number here and on the commit message? thanks

Reviewed all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @Azhng, @kevin-v-ngo, @matthewtodd, and @xinhaoz)

[xinhaoz]

Done.

[Azhng]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @Azhng, @kevin-v-ngo, @matthewtodd, and @xinhaoz)

---

-- commits, line 6 at r1:

Previously, xinhaoz (Xin Hao Zhang) wrote…

As of this change, it will not be possible to view the stmt/txn fingerprint aggregated over the user-specified time range, and there will be multiple entries for a stmt/txn fingerprint (with the specified agg interval start time). I am going off the figma, but @kevin-v-ngo can probably confirm the expected view/behaviour here.

Interesting. I suppose some customer feedback would be helpful here to see whether aggregated or non-aggregated behaviour is preferred, if there's any preference at all.

---

pkg/ui/workspaces/cluster-ui/src/statementDetails/statementDetails.tsx, line 172 at r1 (raw file):

Previously, xinhaoz (Xin Hao Zhang) wrote…

Currently on session details, the active query links to the statement details page for the query based on the URL created from the active query info. Are you saying we will never have collected information for the active query?

Interesting, I didn't know you can redirect from Session Page to Statement Detail Page. Though I know for sure that until the query is done executing, we won't have stats available in the from the API. The result would be a blank page like follow.

[xinhaoz]

---

pkg/ui/workspaces/cluster-ui/src/statementDetails/statementDetails.tsx, line 172 at r1 (raw file):

Previously, Azhng (Archer Zhang) wrote…

Interesting, I didn't know you can redirect from Session Page to Statement Detail Page. Though I know for sure that until the query is done executing, we won't have stats available in the from the API. The result would be a blank page like follow.

I see, that makes sense... In that case, is redirecting from session details active query -> statement details useful? Seems like we would often get results like this. cc @kevin-v-ngo

[kevin-v-ngo]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @Azhng, @maryliag, @matthewtodd, and @xinhaoz)

---

-- commits, line 6 at r1:

Previously, Azhng (Archer Zhang) wrote…

Interesting. I suppose some customer feedback would be helpful here to see whether aggregated or non-aggregated behaviour is preferred, if there's any preference at all.

A good start is non-aggregated. It can help pinpoint outliers at the interval granularity. As we get feedback, it'd be interesting to see if/how we can correlate Start Time with areas such as our time-series metrics.

---

pkg/ui/workspaces/cluster-ui/src/statementDetails/statementDetails.tsx, line 172 at r1 (raw file):

Previously, xinhaoz (Xin Hao Zhang) wrote…

I see, that makes sense... In that case, is redirecting from session details active query -> statement details useful? Seems like we would often get results like this. cc @kevin-v-ngo

We'd only get results if the statement fingerprint has already executed in the most recent interval correct? So this message would only appear during the fingerprint’s first execution within the current aggregation interval coming from the sessions page.

Can we update the text to clarify? "Statement statistics have not yet been collected within the current interval for this statement fingerprint."

[xinhaoz]

---

pkg/ui/workspaces/cluster-ui/src/statementDetails/statementDetails.tsx, line 172 at r1 (raw file):

Previously, kevin-v-ngo wrote…

We'd only get results if the statement fingerprint has already executed in the most recent interval correct? So this message would only appear during the fingerprint’s first execution within the current aggregation interval coming from the sessions page.

Can we update the text to clarify? "Statement statistics have not yet been collected within the current interval for this statement fingerprint."

Spoke to Kevin offline about this and we will be removing the link to statement details from session details in a separate issue.

[kevin-v-ngo]

@Annebirzin and I chatted today and noticed that we should include the Interval Start Time on the Statement details page. This would allow the user to understand which interval they came from and are viewing statistics for. I created a separate issue here but it would be great if we can have this for the V1 experience in 21.2.

[maryliag]

Reviewed 1 of 38 files at r7, 26 of 37 files at r8, 2 of 2 files at r9.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @Azhng, @maryliag, @matthewtodd, and @xinhaoz)

[xinhaoz]

TFTR!
bors r=maryliag

[craig]

Build succeeded:

• GitHub CI (Cockroach)

[blathers-crl]

Encountered an error creating backports. Some common things that can go wrong:

1. The backport branch might have already existed.
2. There was a merge conflict.
3. The backport branch contained merge commits.

You might need to create your backport manually using the backport tool.

---

error creating merge commit from 58bc805 to blathers/backport-release-21.2-70282: 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 21.2.x failed. See errors above.

---

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.}