-
Notifications
You must be signed in to change notification settings - Fork 451
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
[WIP] Extra functionality for queries list page #2534
base: main
Are you sure you want to change the base?
Conversation
Your preview environment pr-2534-bttf has been deployed. Preview environment endpoints are available at: |
b8d6571
to
028976d
Compare
async cancelQuery(externalId: string): Promise<void> { | ||
logger.debug(`Cancel query: ${externalId} - not implemented`); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Getting rid of the abstract definition so we can more easily check whether an integration has a cancelQuery
defined or not
querySource: { | ||
sourceType: "DimensionSlices", | ||
id: this.model.id, | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Checking my understanding here - there's no page to link back to for DimensionSlices, right? I figured it'd be worth storing the source anyway but open to changing that
querySource: { | ||
sourceType: "PastExperiments", | ||
id: this.model.id, | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same question as for DimensionSlices. It seems like a PastExperiments import doesn't have any canonical point to link back to but it might still be worth keeping the source tagged
@@ -86,6 +92,8 @@ const DataSourceQueries = (): React.ReactElement => { | |||
); | |||
} | |||
|
|||
const supportsQueryCancellation = ["athena", "bigquery"].includes(d.type); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there somewhere better that this can live? The source of truth is really whether the backend integration includes a cancelQuery
function so that can't exactly be checked from the frontend code here
@@ -155,6 +164,36 @@ const DataSourceQueries = (): React.ReactElement => { | |||
} | |||
} | |||
|
|||
let LinkToQuerySource = <></>; | |||
if (query.querySource) { | |||
// Backlinks are supported for Metrics and Experiments as they have canonical pages to link to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If there are any other major types I should add support for let me know
apiCall<Response>( | ||
`/datasource/${did}/cancel/${query.id}`, | ||
{ | ||
method: "POST", | ||
} | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could use some input on what to do with the response to this api call. I wasn't sure what the best way to surface an error message or success status to the user would be, as the UI on this page is already getting somewhat cluttered.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this stab at permissions seem right? Or any obvious changes I should make?
e1393ab
to
c7c3325
Compare
Features and Changes
Adds a new column to the Queries page of a datasource for actions to be taken on that query.
Initially there are two possible actions:
Currently, only BigQuery and Athena support cancellation, but we can implement the
cancelQuery
function on more integrations in future PRs.Testing
Run queries from both an experiment & a metric to make sure that the back-linking works correctly.
Screenshots