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

Explore: Reuse Dashboard's QueryRows component #38942

Merged
merged 19 commits into from
Sep 15, 2021
Merged

Conversation

Elfo404
Copy link
Member

@Elfo404 Elfo404 commented Sep 7, 2021

Removes Explore specific implementation and reuse the same QueryRows component from dashboards:
Screenshot 2021-09-14 at 14 12 15

Noticeable behavior differences compared to the previous version:

  1. It's now possible to remove ALL queries
    • This is consistent with dashboards edit, i'd keep this new behavior
  2. Latency info has been removed. It wasn't very useful also given that more detailed performance info are shown in the inspector:
    Screenshot 2021-09-10 at 14 37 27
    Screenshot 2021-09-10 at 14 39 19
  3. pre-emptive log highlight has been removed: this was implemented only by loki, it wasn't a documented feature and it doesn't add much value.
    • What was happening was that the logs panel was highlighting logs from the previously ran query when changing (without running) the current one based on the search expression. we felt this feature is barely used and not very useful. I'll follow up with a PR to deprecate the API in grafana and remove the implementation in Loki
Screen.Recording.2021-09-10.at.14.45.58.mov

Issues

  1. Graphite has a weird behavior (refId changing / editor freezing)
    • This seems to be a graphite problem, @ifrost is investigating
  2. Toggling the second query off and on makes it disappear
    • This happened because when running queries (which happens when toggling one on) we were clearing queries if all of them were empty. this doesn't seem to be useful, so I changed in 426d307 so that they are not cleared anymore now.
    • note: this was also happening before when adding multiple empty queries and then removing one was removing all of them but one.
  3. Removing the query doesn't remove existing logs results
    • Fixed by 98b52d4, queries were being run again after removing a row
  4. After copying the first row, drag & drop becomes super slow and laggy
    • when you start draggin there's always a bit of delay (this also happen in dashboards, even with empty queries). after copying thouh i suspect this is because of the amount of data being processed in the FE that is blocking the UI (either loki processing data or the panel rendering). i can also see it happening and doesn't seem related to the QueryRow component. cc @ifrost

TODO

  • Add tests
  • Ref ids changes are not persisted
  • add deprecation notice for getHighlighterExpressions
    • add deprecation notice for getHighlighterExpressions in changelog
  • add deprecation notice for ExploreQueryFieldProps
    • add deprecation notice for ExploreQueryFieldProps in changelog

Deprecation notice

getHighlighterExpressions in datasource APIs ( used to highlight logs while editing queries) has been deprecated and will be removed in a future release.

Deprecation notice

ExploreQueryFieldProps interface for query editors has been deprecated and will be removed in a future release. Use QueryEditorProps instead.

@ivanahuckova
Copy link
Member

When manually testing this, I've found following inconsistencies with previous QueryRow:

  1. Log highlight - we talked about the fact that this could be probably removed
  2. Removed latency
  3. You are able to remove last query row. In current query row, you are not able to remove last query row.
  4. New query row has drag-drop function to reorder query rows. I was able to end up with this mixture:
  • first row has prometheus name, but graphite editor
  • second row has test data data source
    I think it has something to do with graphite cause I was able to reproduce only with it.
weird.mov
  1. When you have long query and you toggle-hide query row, the query is written in the row and makes whole Explore very long
long.mov

@ifrost
Copy link
Contributor

ifrost commented Sep 10, 2021

A few issues:

Loki

  1. It happened only with empty queries. Toggling the second query off and on makes it disappear:
Screen.Recording.2021-09-10.at.10.50.57.mov

  1. Removing the query doesn't remove existing logs results (maybe the query is not run when a row is removed?)

See how changing visibility changes the graph/results but not removing the row:

Screen.Recording.2021-09-10.at.10.53.31.mov

  1. This seems to be happening only when there's a lot of logs entries rendered (I had ~800). After copying the first row, drag & drop becomes super slow and laggy:
Screen.Recording.2021-09-10.at.11.03.39.mov

Graphite

  1. This might be something Graphite related so I can have a look what's causing it. When a new row is created and the order changes the RefID gets mixed up (see below how ref id changed to "B" in both rows):
Screen.Recording.2021-09-10.at.10.58.36.mov

what's more after switching to a different data source one of the Graphite query remains on the screen.

@Elfo404 Elfo404 force-pushed the gio/feat/shared-query-row branch 2 times, most recently from d416e2a to 6d26546 Compare September 13, 2021 08:46
@torkelo
Copy link
Member

torkelo commented Sep 13, 2021

This PR makes me so happy ❤️

@Elfo404 Elfo404 requested review from ifrost and a team September 14, 2021 13:13
@Elfo404 Elfo404 marked this pull request as ready for review September 14, 2021 13:13
@Elfo404 Elfo404 requested review from marcusolsson and a team as code owners September 14, 2021 13:13
@Elfo404 Elfo404 requested review from a team, andresmgot, vickyyyyyyy, achatterjee-grafana, Eve832, torkelo and natellium and removed request for a team September 14, 2021 13:13
@Elfo404 Elfo404 added the pr/needs-manual-testing Before merge some help with manual testing & verification is requested label Sep 14, 2021
@Elfo404
Copy link
Member Author

Elfo404 commented Sep 14, 2021

Marking this as ready for review.

I'd like it to get in before 8.2 but there are probably a couple of rough edges and some additional love might be needed here and there.

Copy link
Member

@torkelo torkelo left a comment

Choose a reason for hiding this comment

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

Looks great! 🚀

Copy link
Member

@ivanahuckova ivanahuckova left a comment

Choose a reason for hiding this comment

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

Really good job on this!!! 👌 I was still able to reproduce

long.mov

Otherwise LGTM! 🚀

@achatterjee-grafana
Copy link
Contributor

Are there any doc changes or do only the change logs and release notes need an update? Thanks for any clarification.

@Elfo404
Copy link
Member Author

Elfo404 commented Sep 15, 2021

@achatterjee-grafana there's only one docs change in docs/sources/developers/plugins/add-support-for-explore-queries.md , but I guess you were added as a reviewer because you are a codeowner for that file

@achatterjee-grafana
Copy link
Contributor

@achatterjee-grafana there's only one docs change in docs/sources/developers/plugins/add-support-for-explore-queries.md , but I guess you were added as a reviewer because you are a codeowner for that file

Sounds good. I just wanted to make sure we are not missing docs. Thank you!

@Elfo404
Copy link
Member Author

Elfo404 commented Sep 15, 2021

@ivanahuckova I'll merge this one and I'll address the panel length issue in a follow-up pr. Thanks again for catching it!

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.

None yet

6 participants