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

Tempo: Add query ref in the query editor #81343

Merged
merged 5 commits into from
Jan 31, 2024
Merged

Conversation

joey-grafana
Copy link
Contributor

@joey-grafana joey-grafana commented Jan 26, 2024

What is this feature?

Adds a query ref in the Tempo query editor.

Why do we need this feature?

We pass in the query to the Monaco editor but do not also pass updates to the query. So, when the query changes and then the text in the Monaco field (i.e. the TraceQL field) is updated, Monaco will update the Tempo query with the new TraceQL query but unfortunately uses the original Tempo query (without any updates from the user) it was given in the Query Editor onEditorChange and so any changes will not be reflected.

In this PR we add a ref so that Monaco always has the latest values for query.

Who is this feature for?

Tempo users

Which issue(s) does this PR fix?:

Fixes #81192
Fixes #81193

Special notes for your reviewer:

Please check that:

  • It works as expected from a user's perspective.
  • If this is a pre-GA feature, it is behind a feature toggle.
  • The docs are updated, and if this is a notable improvement, it's added to our What's New doc.

@joey-grafana joey-grafana marked this pull request as ready for review January 29, 2024 09:11
@joey-grafana joey-grafana requested a review from a team as a code owner January 29, 2024 09:11
Copy link
Contributor

@fabrizio-grafana fabrizio-grafana left a comment

Choose a reason for hiding this comment

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

Fixes
#81192
#81193

I'm not sure if those fixes will be closed automatically if reported like that. In fact if I open them, I don't see this PR referenced there. I think the correct way is:

Fixes #81192
Fixes #81193

but again, not sure, feel free to ignore the comment!

joey-grafana and others added 2 commits January 29, 2024 14:32
@joey-grafana joey-grafana merged commit b9f5042 into main Jan 31, 2024
13 checks passed
@joey-grafana joey-grafana deleted the joey/tempo-fix-query-ref branch January 31, 2024 09:08
@aocenas
Copy link
Member

aocenas commented Jan 31, 2024

@joey-grafana I don't think this is best way to fix this.

  1. we could and probably should fix it in the CodeEditor itself. This is not even documented but is important caveat to the API. We should either create an issue and let CodeEditor owners handle it or propose a fix.
  2. Even if we weren't going to fix this, the issue is in CodeEditor which is wrapped in TraceQLEditor but we are fixing it in QueryEditor. This is 2 levels separated from the root cause. So anybody who uses TraceQLEditor in some other component would have to first know this is an issue (as this is mentioned nowhere in the TraceQLEditor it's doubtful they will know) and will also need a copy of this same fix. Moving it down a level into TraceQLEditor would at least isolate this issue more.

@joey-grafana
Copy link
Contributor Author

@aocenas yeah sounds good!

  1. Monaco editor does not reflect changes to the query in the onChange callback #81687
  2. Tempo: Move query ref to TraceQLEditor #81686

Ukochka pushed a commit that referenced this pull request Feb 14, 2024
* Add query ref

* Fix lint

* Add comment

* Update public/app/plugins/datasource/tempo/traceql/QueryEditor.tsx

Co-authored-by: Fabrizio <135109076+fabrizio-grafana@users.noreply.github.com>

---------

Co-authored-by: Fabrizio <135109076+fabrizio-grafana@users.noreply.github.com>
@aangelisc aangelisc modified the milestones: 10.4.x, 10.4.0 Mar 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment