Feature/ticketless client tags in query editor #317
Conversation
|
Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla |
|
Cla was accepted waiting for #309 before i can publish this @laserninja @RoeyoOgen |
|
Please squash all commits |
|
@laserninja sadly my code based on your changes, i will pull your changes after you finish the cr process |
RotRotAl
force-pushed
the
feature/ticketless-client-tags-in-query-editor
branch
2 times, most recently
from
904a534 to
c699b9e
Compare
|
@nineinchnick done replying and fixing, thanks for the cr. |
RotRotAl
force-pushed
the
feature/ticketless-client-tags-in-query-editor
branch
from
8bb3509 to
61c6a88
Compare
|
Hi @nineinchnick stil needs second cr 🙏🏻 |
|
@nineinchnick still waits for cr |
nineinchnick
left a comment
nineinchnick
left a comment
There was a problem hiding this comment.
Can you update the PR description with some examples of when this is useful? I'm not sure why this is needed over creating multiple data sources with different client tags configured.
|
I need it in order to give my “clients” the ability to choose in which cluster there query should be running, i give them only the gateway link and they decide by client tag where will it run. @nineinchnick |
|
@nineinchnick waiting for cr 😅 |
|
@laserninja can you perhaps review? |
pkg/trino/datasource-context.go
Outdated
| } | ||
| if queryTags.ClientTags != "" { | ||
| contextWithTags = context.WithValue(contextWithTags, trinoClientTagsKey, queryTags.ClientTags) | ||
| return contextWithTags |
There was a problem hiding this comment.
This only uses client tags from the first query. Can other queries have different client tags set?
There was a problem hiding this comment.
Right.
fixed so all client tags per request are now being joined together.
All clientTags are applied per request rather than per query.
because grafana executes all queries in a panel inside a single QueryData call with a shared context.
There was a problem hiding this comment.
I'm concerned this can produce unexpected results for users when tags set on query A change the results returned by query B.
There was a problem hiding this comment.
it's not possible to apply different tags per query without changing the architecture to send one request per query.
but I agree it can produce unexpected results if queries in the same panel expect different tags.
We could consider a future enhancement where each query executes in its own request context to fully respect per-query tags.
But currently the plugin doesn't support that.
There was a problem hiding this comment.
I understand the concern, and I agree it can produce unexpected results when queries in the same panel expect different tags.
However, I think this can be documented clearly. And that in most use cases, if multiple queries are in the same panel, they are expected to use the same client tags.
There was a problem hiding this comment.
You could validate if the tags are the same and return an error if they're not, but then it would be confusing to users why they're able to configure different tags for every query in the first place.
I don't think documentation solves this.
There was a problem hiding this comment.
One practical solution could be to update the placeholder text for the Client Tags input to clearly indicate that all queries in the panel share the same tags.
it would make the limitation obvious to users.
There was a problem hiding this comment.
Can you try that and make a screenshot so we can see if it's visible enough? Combined with a strict validation, this would be acceptable.
|
Except for the one comment I left above, this looks good. Please squash commits and write good commit messages to prepare it for merging. Also rebase and fix the conflict in e2e tests. |
RotRotAl
force-pushed
the
feature/ticketless-client-tags-in-query-editor
branch
from
4b74897 to
9ba5839
Compare
pkg/trino/datasource-context.go
Outdated
| for i := range req.Queries { | ||
| var queryTags queryClientTag | ||
| if err := json.Unmarshal(req.Queries[i].JSON, &queryTags); err != nil { | ||
| continue |
There was a problem hiding this comment.
Can you add some logging here?
src/e2e.test.ts
Outdated
| async function runQueryAndReturnRequest( | ||
| page: Page, | ||
| clientTag: string | ||
| ): Promise<import('@playwright/test').Request > { |
There was a problem hiding this comment.
Can you remove extra space here?
|
You are checking query-level tags first, then falling back to datasource settings. Consider documenting this behavior or potentially merging tags instead of replacing them entirely. What happens when both datasource-level and query-level tags are set? |
|
Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Jan Waś.
|
RotRotAl
force-pushed
the
feature/ticketless-client-tags-in-query-editor
branch
from
d4b271a to
165235a
Compare
4 participants
Allows to override the client tags that were set in the data source addition , via the the query editor.
Based on pr #309 by @laserninja that allows setting the client tags in the data source addition .