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

ui/cluster-ui: create statements insights api #86596

Merged
merged 1 commit into from
Aug 25, 2022

Conversation

xinhaoz
Copy link
Member

@xinhaoz xinhaoz commented Aug 22, 2022

This commit adds a function to the cluster-ui pkg that queries
crdb_internal.cluster_execution_insights to surface slow running
queries. The information retrieved is intended to be used in the
insights statement overview and details pages. A new field
statementInsights is added to the cachedData object in the
db-console redux store, and the corresponding function to issue the
data fetch is also added in apiReducers. This commit does not add
any reducers or sagas to CC to fetch and store this data.

Release justification: non-production code change
Release note: None

@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Contributor

@maryliag maryliag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @j82w and @xinhaoz)


pkg/ui/workspaces/cluster-ui/src/api/insightsApi.ts line 249 at r1 (raw file):

  session_id: string;
  txn_id: string;
  txn_fingerprint_id: string; // bytes

we are always using the ids as int64, so it's better to keep consistency, in case we want to use that value to link to the details page of that fingerprint id. To be able to do this there are 2 things you will need to change. I'll add a line on the 2 places


pkg/ui/workspaces/cluster-ui/src/api/insightsApi.ts line 282 at r1 (raw file):

    const end = moment.utc(row.end_time);
    return {
      transactionID: row.txn_id,

are you missing txn fingerprint ID? Looks like you can on the query, but don't use it


pkg/ui/workspaces/cluster-ui/src/api/insightsApi.ts line 291 at r1 (raw file):

      execType: InsightExecEnum.STATEMENT,
      statementID: row.stmt_id,
      statementFingerprintID: row.stmt_fingerprint_id,

use HexStringToInt64String(row.stmt_fingerprint_id)


pkg/ui/workspaces/cluster-ui/src/api/insightsApi.ts line 314 at r1 (raw file):

      session_id,
      txn_id,
      txn_fingerprint_id,

change to encode(txn_fingerprint_id, 'hex') AS txn_fingerprint_id,


pkg/ui/workspaces/cluster-ui/src/api/insightsApi.ts line 316 at r1 (raw file):

      txn_fingerprint_id,
      stmt_id,
      stmt_fingerprint_id,

change to encode(stmt_fingerprint_id, 'hex') AS stmt_fingerprint_id,

Copy link
Member Author

@xinhaoz xinhaoz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @j82w and @maryliag)


pkg/ui/workspaces/cluster-ui/src/api/insightsApi.ts line 249 at r1 (raw file):

Previously, maryliag (Marylia Gutierrez) wrote…

we are always using the ids as int64, so it's better to keep consistency, in case we want to use that value to link to the details page of that fingerprint id. To be able to do this there are 2 things you will need to change. I'll add a line on the 2 places

Done.


pkg/ui/workspaces/cluster-ui/src/api/insightsApi.ts line 282 at r1 (raw file):

Previously, maryliag (Marylia Gutierrez) wrote…

are you missing txn fingerprint ID? Looks like you can on the query, but don't use it

Done.


pkg/ui/workspaces/cluster-ui/src/api/insightsApi.ts line 291 at r1 (raw file):

Previously, maryliag (Marylia Gutierrez) wrote…

use HexStringToInt64String(row.stmt_fingerprint_id)

Done.


pkg/ui/workspaces/cluster-ui/src/api/insightsApi.ts line 314 at r1 (raw file):

Previously, maryliag (Marylia Gutierrez) wrote…

change to encode(txn_fingerprint_id, 'hex') AS txn_fingerprint_id,

Done.


pkg/ui/workspaces/cluster-ui/src/api/insightsApi.ts line 316 at r1 (raw file):

Previously, maryliag (Marylia Gutierrez) wrote…

change to encode(stmt_fingerprint_id, 'hex') AS stmt_fingerprint_id,

Done.

Copy link
Contributor

@maryliag maryliag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 2 of 2 files at r2.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @j82w and @xinhaoz)


pkg/ui/workspaces/cluster-ui/src/api/insightsApi.ts line 249 at r1 (raw file):

Previously, xinhaoz (Xin Hao Zhang) wrote…

Done.

don't forget to remove the bytes comment, since it's no longer bytes

@xinhaoz xinhaoz force-pushed the insights-api branch 2 times, most recently from fbf7be0 to f7fb31a Compare August 22, 2022 20:57
@xinhaoz xinhaoz marked this pull request as ready for review August 22, 2022 20:58
@xinhaoz xinhaoz requested a review from a team August 22, 2022 20:58
Copy link
Contributor

@maryliag maryliag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 4 of 6 files at r1, 1 of 1 files at r3, all commit messages.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @j82w)

Copy link
Contributor

@ericharmeling ericharmeling left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! LGTM

Reviewed all commit messages.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @j82w)

Copy link
Contributor

@j82w j82w left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @xinhaoz)


pkg/ui/workspaces/cluster-ui/src/api/insightsApi.ts line 308 at r4 (raw file):

}

const statemenInsightsQuery: InsightQuery<

typo: statementInsightsQuery

Copy link
Contributor

@j82w j82w left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @xinhaoz)


pkg/ui/workspaces/cluster-ui/src/api/insightsApi.ts line 290 at r4 (raw file):

      endTime: end,
      databaseName: row.database_name,
      elapsedTimeMillis: start.diff(end, "milliseconds"),

This causes it to be negative. You need to reverse start and end.

Copy link
Contributor

@j82w j82w left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (and 1 stale) (waiting on @maryliag)

@xinhaoz
Copy link
Member Author

xinhaoz commented Aug 23, 2022

TFTR!
bors r+

@craig
Copy link
Contributor

craig bot commented Aug 23, 2022

Build failed (retrying...):

@craig
Copy link
Contributor

craig bot commented Aug 23, 2022

Build failed:

@xinhaoz
Copy link
Member Author

xinhaoz commented Aug 23, 2022

bors r+

@craig
Copy link
Contributor

craig bot commented Aug 23, 2022

Build failed (retrying...):

@craig
Copy link
Contributor

craig bot commented Aug 24, 2022

Build failed:

@j82w
Copy link
Contributor

j82w commented Aug 24, 2022

bors r+

@craig
Copy link
Contributor

craig bot commented Aug 24, 2022

Build failed (retrying...):

This commit adds a function to the `cluster-ui` pkg that queries
`crdb_internal.cluster_execution_insights` to surface slow running
queries. The information retrieved  is intended to be used in the
insights statement overview and details pages. A new field
`statementInsights` is added to the `cachedData` object in the
db-console redux store, and the corresponding function to issue the
data fetch is also added in `apiReducers`. This commit does not add
any reducers  or sagas to CC to fetch and store this data.

Release justification: non-production code change
Release note: None
@craig
Copy link
Contributor

craig bot commented Aug 24, 2022

Canceled.

@xinhaoz
Copy link
Member Author

xinhaoz commented Aug 24, 2022

bors r+

@craig
Copy link
Contributor

craig bot commented Aug 24, 2022

Build failed (retrying...):

@craig
Copy link
Contributor

craig bot commented Aug 25, 2022

Build succeeded:

@craig craig bot merged commit af9d88c into cockroachdb:master Aug 25, 2022
@xinhaoz xinhaoz deleted the insights-api branch September 21, 2022 21:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants