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

[WIP] Extra functionality for queries list page #2534

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

Kevin-Chant
Copy link
Contributor

@Kevin-Chant Kevin-Chant commented May 16, 2024

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:

  • Visit the experiment or metric responsible for triggering that query
  • Cancel a running or queued query (if the datasource supports cancellation and the user has appropriate permissions)

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

image
image

Copy link

github-actions bot commented May 16, 2024

Your preview environment pr-2534-bttf has been deployed.

Preview environment endpoints are available at:

@Kevin-Chant Kevin-Chant force-pushed the kc/queries-page-extra-functionality branch from b8d6571 to 028976d Compare May 16, 2024 21:08
Comment on lines -120 to -122
async cancelQuery(externalId: string): Promise<void> {
logger.debug(`Cancel query: ${externalId} - not implemented`);
}
Copy link
Contributor Author

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

Comment on lines +62 to +65
querySource: {
sourceType: "DimensionSlices",
id: this.model.id,
},
Copy link
Contributor Author

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

Comment on lines +46 to +49
querySource: {
sourceType: "PastExperiments",
id: this.model.id,
},
Copy link
Contributor Author

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);
Copy link
Contributor Author

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
Copy link
Contributor Author

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

Comment on lines +267 to +272
apiCall<Response>(
`/datasource/${did}/cancel/${query.id}`,
{
method: "POST",
}
);
Copy link
Contributor Author

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.

Copy link
Contributor Author

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?

@Kevin-Chant Kevin-Chant force-pushed the kc/queries-page-extra-functionality branch from e1393ab to c7c3325 Compare May 21, 2024 19: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.

None yet

1 participant