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

LogsVolumePanel: support filtering logs from the selected level #474

Merged
merged 30 commits into from
Jul 26, 2024

Conversation

matyax
Copy link
Contributor

@matyax matyax commented Jun 20, 2024

Closes #422

This PR adds support to apply level filters from the Logs Volume panel.

Demo.mov

@matyax
Copy link
Contributor Author

matyax commented Jun 20, 2024

There's one scenario that we need to consider: the user previously had applied a level filter.

Interacting with the Logs Volume panel adds detected_level filters. Options:

  • Ignore: do nothing, add a detected_level filter
  • Remove the level filter before adding detected_level
  • Change the existing level filter

@matyax
Copy link
Contributor Author

matyax commented Jun 20, 2024

When the user is appending levels, we don't change the filters because we don't yet support filters with multiple values.

@matyax matyax self-assigned this Jun 21, 2024
@matyax matyax force-pushed the matyax/logs-volume-filtering branch from 745b5b1 to 12c3dff Compare June 21, 2024 11:31
Copy link
Collaborator

@cyriltovena cyriltovena left a comment

Choose a reason for hiding this comment

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

LGTM

@matyax
Copy link
Contributor Author

matyax commented Jun 24, 2024

Thanks @cyriltovena ! What does @grafana/observability-logs / @zizzpudding this of this? How does this behavior feels to you?

@gtk-grafana
Copy link
Contributor

@matyax what if we just ignored level/detected_level in the logs volume, although then we'd want to be able to set legend visibility when a level/detected_level filter is applied, but it looks like we can do that with series visibility overrides? 🤔

"matcher": {
          "id": "byNames",
          "options": {
            "mode": "exclude",
            "names": [
              "A-series1"
            ],
            "prefix": "All except:",
            "readOnly": true
          }
        },

@gtk-grafana gtk-grafana marked this pull request as draft June 26, 2024 16:21
auto-merge was automatically disabled June 26, 2024 16:21

Pull request was converted to draft

@cyriltovena
Copy link
Collaborator

I haven't follow the tech side of it but I love this feature.

@matyax matyax force-pushed the matyax/logs-volume-filtering branch 2 times, most recently from 8b04be1 to 2ef107b Compare July 12, 2024 10:18
@matyax matyax force-pushed the matyax/logs-volume-filtering branch from 2ef107b to 2edfb16 Compare July 19, 2024 08:43
@matyax matyax marked this pull request as ready for review July 19, 2024 08:55
@matyax matyax force-pushed the matyax/logs-volume-filtering branch from e11be15 to c182f22 Compare July 19, 2024 16:03
@matyax
Copy link
Contributor Author

matyax commented Jul 19, 2024

I've added a new variable, dedicated to hold the level, that is used everywhere but in the logs volume query. This should take this feature pretty much where we wanted.

src/services/panel.ts Outdated Show resolved Hide resolved
@matyax matyax force-pushed the matyax/logs-volume-filtering branch from 55e566f to 0e0cafd Compare July 22, 2024 10:13
@matyax
Copy link
Contributor Author

matyax commented Jul 22, 2024

Not having to refresh logs volume seems to produce a bit more of responsiveness in the app. And it's one less query that we need to run. 💡

@matyax matyax requested a review from gtk-grafana July 22, 2024 16:32
@matyax
Copy link
Contributor Author

matyax commented Jul 22, 2024

@grafana/observability-logs Mind taking another look? Thanks!

@matyax
Copy link
Contributor Author

matyax commented Jul 22, 2024

It seems that we've been bitten by the buggy interpolation error we recently fixed, as the metric query it's producing for for fields and values is like: sum(count_over_time({service_name=tempo-distributor} | logfmt {service_name=\"tempo-distributor\"}| drop __error__ | namespace!=\"\" [$__auto])) by (namespace)

@@ -12,7 +14,14 @@ export const VAR_LOGS_FORMAT = 'logsFormat';
export const VAR_LOGS_FORMAT_EXPR = '${logsFormat}';
export const VAR_LINE_FILTER = 'lineFilter';
export const VAR_LINE_FILTER_EXPR = '${lineFilter}';
export const LOG_STREAM_SELECTOR_EXPR = `${VAR_LABELS_EXPR} ${VAR_PATTERNS_EXPR} ${VAR_LOGS_FORMAT_EXPR} ${VAR_FIELDS_EXPR} ${VAR_LINE_FILTER_EXPR}`;
let STREAM_SELECTOR_EXPR = `${VAR_LABELS_EXPR} ${VAR_PATTERNS_EXPR} ${VAR_LOGS_FORMAT_EXPR} ${VAR_LEVELS_EXPR} ${VAR_FIELDS_EXPR} ${VAR_LINE_FILTER_EXPR}`;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We either hold this PR until a new Grafana image is available, or we do something like this. Any suggestion will be much appreciated.

To reproduce, use the app with yarn server (as the e2e tests do), and browse labels.

imagen

@matyax
Copy link
Contributor Author

matyax commented Jul 23, 2024

We will wait for a new Grafana release before merging these changes.

@matyax matyax force-pushed the matyax/logs-volume-filtering branch from 7795455 to 6b6f988 Compare July 25, 2024 17:15
@matyax matyax enabled auto-merge (squash) July 26, 2024 09:25
@matyax matyax disabled auto-merge July 26, 2024 10:20
@matyax matyax enabled auto-merge (squash) July 26, 2024 10:40
@matyax
Copy link
Contributor Author

matyax commented Jul 26, 2024

Never stop believing.

@matyax matyax merged commit c2c611e into main Jul 26, 2024
3 of 4 checks passed
@svennergr
Copy link
Contributor

I think with that we should also upgrade the grafana versions in plugin.json and docker-compose.yaml, WDYT?

@matyax
Copy link
Contributor Author

matyax commented Jul 26, 2024

Definitely.

gtk-grafana added a commit that referenced this pull request Jul 26, 2024
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.

Logs: allow filtering from logs volume
4 participants