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: move events endpoint to use sql-over-http #91745

Merged
merged 1 commit into from
Nov 21, 2022

Conversation

THardy98
Copy link

@THardy98 THardy98 commented Nov 11, 2022

Part of: #89429

Addresses: #90272 (blocked from resolving by #80789)

Demo (DB-Console only): https://www.loom.com/share/bb67044a8560407fb703af2a6bbb2d9a

This change migrates the existing /events request to use the sql-over-http endpoint on apiV2, making this request tenant-scoped once the sql-over-http endpoint is scoped to tenants (this should be the case when #91323 is completed).

Release note: None

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@THardy98 THardy98 requested a review from a team November 11, 2022 15:03
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.

can you add a video showing the pages working with the new changes, like you did on your other PRs?

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained

@THardy98 THardy98 force-pushed the sql_api_events_endpoint branch 2 times, most recently from 1285d71 to 5303a62 Compare November 15, 2022 16:20
Copy link
Author

@THardy98 THardy98 left a comment

Choose a reason for hiding this comment

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

Added a demo :)

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained

@THardy98 THardy98 force-pushed the sql_api_events_endpoint branch from 5303a62 to d927be5 Compare November 15, 2022 18:14
@THardy98 THardy98 requested review from maryliag and a team November 15, 2022 18:15
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 3 of 7 files at r1, 9 of 9 files at r2, all commit messages.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @THardy98)


pkg/ui/workspaces/cluster-ui/src/api/eventsApi.ts line 21 at r2 (raw file):

// defaultEventsMaxBytesLimit is the default max size of events payload in bytes.
export const defaultEventsMaxBytesLimit = 50000; // 50KB

I think I see a similar variable with the same size being used in a few other place, can you create a global const and replace the usages?


pkg/ui/workspaces/cluster-ui/src/api/eventsApi.ts line 71 at r2 (raw file):

  }
  eventsStmt.sql += ";";
  console.log("EVENTS STMT", eventsStmt);

nit: remove this line

Code quote:

SELECT timestamp, "eventType", "reportingID", info, "uniqueID" FROM system.eventlog

pkg/ui/workspaces/cluster-ui/src/api/eventsApi.ts line 86 at r2 (raw file):

}

// getEvents fetches events logs from the database. Callers of

nit: s/getEvents/getNonRedactedEvents


pkg/ui/workspaces/cluster-ui/src/api/eventsApi.ts line 87 at r2 (raw file):

// getEvents fetches events logs from the database. Callers of
// getEvents from cluster-ui will need to pass a timeout argument for

nit: s/getEvents/getNonRedactedEvents


pkg/ui/workspaces/db-console/src/redux/events.ts line 17 at r2 (raw file):

 */
export function eventsSelector(state: AdminUIState) {
  return state.cachedData.events.data && state.cachedData.events.data.events;

curious: why this is no longer needed?

@THardy98 THardy98 force-pushed the sql_api_events_endpoint branch from d927be5 to 331e667 Compare November 21, 2022 15:31
Copy link
Author

@THardy98 THardy98 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 @maryliag)


pkg/ui/workspaces/cluster-ui/src/api/eventsApi.ts line 21 at r2 (raw file):

Previously, maryliag (Marylia Gutierrez) wrote…

I think I see a similar variable with the same size being used in a few other place, can you create a global const and replace the usages?

Yup, LARGE_RESULT_SIZE already existed in sqlApi, using that instead.


pkg/ui/workspaces/cluster-ui/src/api/eventsApi.ts line 71 at r2 (raw file):

Previously, maryliag (Marylia Gutierrez) wrote…

nit: remove this line

Done.


pkg/ui/workspaces/cluster-ui/src/api/eventsApi.ts line 86 at r2 (raw file):

Previously, maryliag (Marylia Gutierrez) wrote…

nit: s/getEvents/getNonRedactedEvents

Done.


pkg/ui/workspaces/cluster-ui/src/api/eventsApi.ts line 87 at r2 (raw file):

Previously, maryliag (Marylia Gutierrez) wrote…

nit: s/getEvents/getNonRedactedEvents

Done.


pkg/ui/workspaces/db-console/src/redux/events.ts line 17 at r2 (raw file):

Previously, maryliag (Marylia Gutierrez) wrote…

curious: why this is no longer needed?

The type of the response changed to the new EventsResponse type defined in eventsApi. The new type doesn't have an events field, it's just a list of EventsColumns:

export type EventsResponse = EventColumns[];

so we don't need the && check anymore.

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 1 of 1 files at r3, all commit messages.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @THardy98)

Part of: cockroachdb#89429

Addresses: cockroachdb#90272 (blocked from resolving by cockroachdb#80789)

This change migrates the existing `/events` request to use the
sql-over-http endpoint on apiV2, making this request tenant-scoped once
the sql-over-http endpoint is scoped to tenants (this should be the case
when cockroachdb#91323 is completed).

Release note: None
@THardy98 THardy98 force-pushed the sql_api_events_endpoint branch from 331e667 to a68cd89 Compare November 21, 2022 15:45
@THardy98
Copy link
Author

TYFR

@THardy98
Copy link
Author

bors r+

@craig
Copy link
Contributor

craig bot commented Nov 21, 2022

Build succeeded:

@craig craig bot merged commit df51702 into cockroachdb:master Nov 21, 2022
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.

3 participants