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

Prometheus: Refresh query field metrics on data source change #24116

Merged
merged 1 commit into from
May 4, 2020

Conversation

s-h-a-d-o-w
Copy link
Contributor

What this PR does / why we need it:
Addresses metrics chooser not refreshing when switching from one Prometheus data source to another (dashboard only).

Which issue(s) this PR fixes:
Fixes #23162

Special notes for your reviewer:
Funnily enough, the reason why this problem didn't occur in Explore was that the query fields got unmounted and "remounted" seemingly unnecessarily. While in Dashboard, they stay mounted.

And so the ideal way to resolve this seemed to me to have PromQueryField refresh the metrics if it sees that languageProvider changed in componentDidUpdate and fix the unnecessary unmounting in Explore, so that the behavior of Dashboard and Explore become consistent in this respect.

As an aside, since storing of props in class fields is generally inadvisable and I didn't see a reason for why it would be necessary here, I refactored this and restructured things a bit so componentDidMount and componentDidUpdate can be more succinct. Of course please let me know if you would rather not have me include this in this PR.

... `componentDidUpdate`, not just `componentDidMount`.
Also unify query field behavior of Explore with Dashboard - when the
data source changes, it doesn't unmount but instead refreshes its
metrics.

Fixes #23162
Copy link
Contributor

@davkal davkal left a comment

Choose a reason for hiding this comment

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

Yeah, we have previously used key changes to wipe query rows because of state issues with the input fields, but it seems like they no longer happen. Since we don't allow ordering or filtering of rows, I think using the index is fine for now (even though it might trip us up later if ordering gets implemented).

The issue itself is nicely fixed by this PR. LGTM

@s-h-a-d-o-w
Copy link
Contributor Author

even though it might trip us up later if ordering gets implemented

In that case, I would consider a change of the ordering implementation. Keeping a separate index list for an array that is never changed may be great for high performance applications with huge arrays but for a few elements like we have here, I'd go with the improved robustness and readability of simply reordering the query array itself. (Possible caveat: I'm not aware of the bigger picture here yet, so maybe there is something that would make that a bad idea.)

@s-h-a-d-o-w s-h-a-d-o-w merged commit 827f99f into master May 4, 2020
@s-h-a-d-o-w s-h-a-d-o-w deleted the aop/refresh-prom-query-field-metrics branch May 4, 2020 08:17
aknuds1 pushed a commit that referenced this pull request May 7, 2020
... in `componentDidUpdate`, not just `componentDidMount`.

Also unify query field behavior of Explore with Dashboard - when the
data source changes, it doesn't unmount but instead refreshes its
metrics.

Fixes #23162

(cherry picked from commit 827f99f)
aknuds1 pushed a commit that referenced this pull request May 7, 2020
... in `componentDidUpdate`, not just `componentDidMount`.

Also unify query field behavior of Explore with Dashboard - when the
data source changes, it doesn't unmount but instead refreshes its
metrics.

Fixes #23162

(cherry picked from commit 827f99f)
aknuds1 pushed a commit that referenced this pull request Jun 30, 2020
... in `componentDidUpdate`, not just `componentDidMount`.

Also unify query field behavior of Explore with Dashboard - when the
data source changes, it doesn't unmount but instead refreshes its
metrics.

Fixes #23162
@ying-jeanne ying-jeanne added the pr/external This PR is from external contributor label Apr 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Prometheus metrics chooser does not refresh when datasource is changed
5 participants