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.

Sign up for GitHub

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

Jump to bottom

Loki/Prometheus: Fix wrong queries executed in split view #60172

Merged
merged 4 commits into from Dec 12, 2022
Merged

Loki/Prometheus: Fix wrong queries executed in split view #60172

merged 4 commits into from Dec 12, 2022

Conversation

svennergr
Copy link
Contributor

What is this feature?

Currently the wrong query is executed when multiple Monaco editors are present. This PR adds a context value, which has to be true, to the relevant editor.addCommand call.

Which issue(s) does this PR fix?:

Fixes #60059

@svennergr svennergr added this to the 9.4.0 milestone Dec 12, 2022
@svennergr svennergr self-assigned this Dec 12, 2022
@svennergr svennergr requested review from a team as code owners December 12, 2022 15:23
@svennergr
Copy link
Contributor Author

svennergr commented Dec 12, 2022 •
edited

@gtk-grafana / @matyax before adding tests to this, how do we feel about fixing it like this? I haven't figured out a better way.

@matyax
Copy link
Contributor

matyax commented Dec 12, 2022

how do we feel about fixing it like this?

At first glance, I don't like this approach, which seems to resolve the issue in a hacky way. Since I can't see right now the root cause of the bug, that's all the feedback I have.

@matyax
Copy link
Contributor

matyax commented Dec 12, 2022

BTW, unfortunately you would not be able to add a regression test for this bug because the editor doesn't render in React tests.

@svennergr
Copy link
Contributor Author

how do we feel about fixing it like this?

At first glance, I don't like this approach, which seems to resolve the issue in a hacky way. Since I can't see right now the root cause of the bug, that's all the feedback I have.

Btw, I was able to reproduce this with 2 editors in the Monaco Editor Playground.

@svennergr
Copy link
Contributor Author

svennergr commented Dec 12, 2022 •
edited

Yea, there are several issues reported to https://github.com/microsoft/monaco-editor: https://github.com/microsoft/monaco-editor/search?q=addCommand+instance&type=issues

With microsoft/monaco-editor#2947 being the first one since February.

So, we could either downgrade to Monaco 0.31.x or use this fix I'd say.

@matyax
Copy link
Contributor

matyax commented Dec 12, 2022

I was just looking there 👀

@matyax
Copy link
Contributor

matyax commented Dec 12, 2022

If this solves the issue, let's go with this. Nice work 🤝

matyax
matyax approved these changes Dec 12, 2022
View reviewed changes
Copy link
Contributor

@matyax matyax left a comment

Choose a reason for hiding this comment

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

🥇

@matyax
Copy link
Contributor

matyax commented Dec 12, 2022

Locally verified.

@svennergr
Copy link
Contributor Author

Locally verified.

Thanks Matías! Going to wait for a ✅ from @gtk-grafana / @grafana/observability-metrics and then merge it.

@svennergr svennergr modified the milestones: 9.4.0, 9.3.2 Dec 12, 2022
gtk-grafana
gtk-grafana approved these changes Dec 12, 2022
View reviewed changes
Copy link
Contributor

@gtk-grafana gtk-grafana left a comment

Choose a reason for hiding this comment

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

LGTM, the fix is working locally for me. Thanks for digging into this!

@svennergr svennergr changed the title Loki/Prometheus: Fix a bug where the wrong query is executed in split view Loki/Prometheus: Fix wrong queries executed in split view Dec 12, 2022
@svennergr svennergr merged commit 8356df0 into main Dec 12, 2022
@svennergr svennergr deleted the svennergr/fix-multiple-monaco-editor branch December 12, 2022 16:50
@grafanabot grafanabot mentioned this pull request Dec 12, 2022
Merged
grafanabot pushed a commit that referenced this pull request Dec 12, 2022
@svennergr @grafanabot
Loki/Prometheus: Fix wrong queries executed in split view (#60172)
948385f 
* add context to monaco editors

(cherry picked from commit 8356df0)
svennergr added a commit that referenced this pull request Dec 12, 2022
@grafanabot @svennergr
[v9.3.x] Loki/Prometheus: Fix wrong queries executed in split view (#…
ef5286d 
…60184)

Loki/Prometheus: Fix wrong queries executed in split view (#60172)

* add context to monaco editors

(cherry picked from commit 8356df0)

Co-authored-by: Sven Grossmann <sven.grossmann@grafana.com>
@TheUnlocked TheUnlocked mentioned this pull request Feb 3, 2023
Open
2 tasks
GuYounes pushed a commit to paul-wurth/BIXpert that referenced this pull request Feb 8, 2023
@grafanabot @svennergr @GuaYounesPW
[v9.3.x] Loki/Prometheus: Fix wrong queries executed in split view (g…
55ae112 
…rafana#60184)

Loki/Prometheus: Fix wrong queries executed in split view (grafana#60172)

* add context to monaco editors

(cherry picked from commit 8356df0)

Co-authored-by: Sven Grossmann <sven.grossmann@grafana.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@matyax matyax matyax approved these changes

@gtk-grafana gtk-grafana gtk-grafana approved these changes

Assignees

@svennergr svennergr

Labels
add to changelog area/frontend backport v9.3.x datasource/Loki datasource/Prometheus enterprise-ok type/bug
Projects
None yet
Milestone
9.3.2
Development

Successfully merging this pull request may close these issues.

Explore: After split screen, Shift-Enter in left pane triggers query evaluation in right pane!
4 participants
@svennergr @matyax @gtk-grafana @grafanabot