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

feat(detected-fields): use /detected_fields API #736

Merged
merged 98 commits into from
Sep 10, 2024

Conversation

svennergr
Copy link
Contributor

@svennergr svennergr commented Aug 28, 2024

Adds support for structured metadata using the detected_fields API endpoint.

Notes for your reviewer:
Firstly, I'm sorry for the size of this PR.

This PR changes a bunch of how the application works.

  • The parser variable is no longer used
    • except as a hack to get the log context query working (todo open issue in grafana/grafana)
  • We no longer have a concept of "this response is json or logfmt or mixed"
    • Instead each field is checked against the response in detected_fields, and we use the parser from there, if multiple parsers are defined in the response, we'll use mixed
  • If a field is included in a query, we will use the appropriate parser
  • If multiple fields, with multiple parsers are included in a query, we will use mixed
    • Unless structured metadata and single other parser is used, then we should just use one
  • Queries should be overall more optimal, line-filters and patterns are now coming before the parsers
    • We should only ever use the minimum required set of parsers
      • The exception is the patterns and log panel queries, which will always use both parsers so the list of labels is consistent between detected_fields (also getting both parsers)
  • Bug fixes
    • memoization in sorting was causing some time series queries to run, but the panels would never update
    • Aggregated field breakdown was re-rendering all panels, causing queries to get cancelled and then re-inited, this should be fixed now as well (but still a problem in the labels, todo create follow up issue)
    • When deleting content out of the line-filter, the empty line filter would still be included in the query unless the user clicked the "x". Now just deleting the content removes the line-filter from all queries
  • New features
    • Since the detected_fields is sorted by cardinality desc, we pushed up all the non-performant time-series to the top, which was eating up browser resources
      • Pulled in the UI from explore to only show 20 series by default, and allow the user to manually show more
  • e2e rehaul:
    • Tracked down flaking tests, and updated how we block queries to help speed things up and reduce strain on the testing loki instance
    • Added a bunch of tests, some json coverage, new helper methods, and more!

@gtk-grafana
Copy link
Contributor

gtk-grafana commented Sep 5, 2024

e2e tests are broken against grafana/loki:main-aa1ac99 due to grafana/loki#14053, but with a local loki I can confirm e2e tests are working.

This should pass e2e tests after we update the docker compose files with a loki version containing a fix for grafana/loki#14053 and grafana/loki#14063.

@matyax
Copy link
Contributor

matyax commented Sep 5, 2024

Log Context is working 👌

@gtk-grafana gtk-grafana marked this pull request as ready for review September 10, 2024 13:43
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.

👏 👏

@gtk-grafana gtk-grafana merged commit 283e4c7 into main Sep 10, 2024
4 checks passed
@gtk-grafana gtk-grafana deleted the svennergr/detected-fields-new branch September 10, 2024 19:21
@trevorwhitney
Copy link
Contributor

yay 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants