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

Loki: Always display log context toggle #66379

Merged
merged 3 commits into from Apr 19, 2023

Conversation

matyax
Copy link
Contributor

@matyax matyax commented Apr 12, 2023

The changes in this PR are to always offer Logs Context, independent of the current query.

The reason for this is that log context is created from all labels defining the stream for the selected log line. And it has basically nothing to do with filter being present/not being present. I think that showing log context only when filter is present is actually more confusing as it makes log context associated with line filters, when it should be associated with log line labels/streams.

This PR adds support for offering logs context for logs lines generated using negative filters.

Since deciding to show or not show the logs context toggle depends on the presence of search words https://github.com/grafana/grafana/blob/main/public/app/plugins/datasource/loki/datasource.ts#L821 ; and search words were based on the presence of PipeExact or PipeMatch nodes, the code has been changed to also look for Neq and Nre nodes. When these are present, the search words are prepended with -. For example, != word is processed as -word.

The other alternative was adding a new function to look for Neq and Nre in the query, and add a new custom property returned from the backendResultTransformer as, for example, custom.hasNegativeExpressions https://github.com/grafana/grafana/blob/main/public/app/plugins/datasource/loki/backendResultTransformer.ts#L4 ; and then updating logsSeriesToLogsModels and the LogsModel type, to pick up this property and return it for every row. https://github.com/grafana/grafana/blob/main/public/app/core/logsModel.ts#L426

I decided for the alternative I sent in this PR because it was simpler and didn't require more parsing, new properties, or type updates.

Which issue(s) does this PR fix?:

Fixes #66132

Special notes for your reviewer:

Returning negative expressions as negative search words didn't seem to have any visible harmful effect, and allowed us to offer logs context for these queries. Nothing should be broken after these changes.

To test, write any query with negative expressions and see the logs context toggle for each row:

Now we will always display the button for logs.

@matyax matyax requested a review from a team as a code owner April 12, 2023 13:50
@matyax matyax added add to changelog no-backport Skip backport of PR no-changelog Skip including change in changelog/release notes and removed add to changelog labels Apr 12, 2023
@matyax matyax added this to the 10.0.0 milestone Apr 12, 2023
@github-actions
Copy link
Contributor

Backend code coverage report for PR #66379
No changes

@github-actions
Copy link
Contributor

github-actions bot commented Apr 12, 2023

Frontend code coverage report for PR #66379

Plugin Main PR Difference
loki 84.62% 84.64% .02%

@matyax matyax modified the milestone: 10.0.0 Apr 12, 2023
@matyax matyax requested review from a team and removed request for a team April 17, 2023 08:14
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.

I think that to fix #66132, we should consider always showing log context. And the more I am thinking about it, the more I think it make sense to always show log context toggle.

The reason for this is that log context is created from all labels defining the stream for the selected log line. And it has basically nothing to do with filter being present/not being present. I think that showing log context only when filter is present is actually more confusing as it makes log context associated with line filters, when it should be associated with log line labels/streams.

So before moving with this PR that introduces negative searchWords (that still acts in Highlight component like normal searchWords because - has no effect on showing/not showing highlighted text), I would consider just showing context all the time.

What do you think @grafana/observability-logs?

@svennergr
Copy link
Contributor

What do you think https://github.com/orgs/grafana/teams/observability-logs?

Makes sense to me. Should we do it before G10?

@matyax
Copy link
Contributor Author

matyax commented Apr 18, 2023

Thank you @ivanahuckova. Always showing the context button makes sense to me as well 👍

I can do that in this PR if we all agree.

@gabor
Copy link
Contributor

gabor commented Apr 18, 2023

What do you think @grafana/observability-logs?

👍

@matyax matyax changed the title Loki search words: Process negative expressions as negative search words Logs Context: Always display context toggle Apr 18, 2023
@matyax matyax force-pushed the matyax/logs-context-negative-expressions branch from f972ef8 to d875b25 Compare April 18, 2023 14:44
@matyax
Copy link
Contributor Author

matyax commented Apr 18, 2023

@svennergr One thing I noticed after this is that it seems to be ignoring the log line immediately above the one I'm expanding:

imagen

imagen

@matyax matyax requested review from a team and ivanahuckova April 18, 2023 14:45
@svennergr
Copy link
Contributor

@svennergr One thing I noticed after this is that it seems to be ignoring the log line immediately above the one I'm expanding:

I guess that logline has different labels than the others. You can set the logsContextDatasourceUi feature flag and remove some labels. It should appear at one point.

Copy link
Contributor

@svennergr svennergr left a comment

Choose a reason for hiding this comment

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

LGTM.

I am wondering if we should mark showContextToggle here https://github.com/grafana/grafana/blob/0457291ac2cfc987985c805e0f03282861c695e9/packages/grafana-data/src/types/logs.ts deprecated and remove that functionality at one point completely.

@matyax
Copy link
Contributor Author

matyax commented Apr 19, 2023

I think we should, @svennergr , as every showContextToggle I see is just returning true. #66819

@matyax matyax enabled auto-merge (squash) April 19, 2023 08:57
@matyax matyax merged commit b47b879 into main Apr 19, 2023
14 checks passed
@matyax matyax deleted the matyax/logs-context-negative-expressions branch April 19, 2023 09:03
ryantxu pushed a commit that referenced this pull request Apr 19, 2023
* Loki search words: process negative expressions as negative search words

* Revert "Loki search words: process negative expressions as negative search words"

This reverts commit d875b25.

* Logs Context: always display
@ivanahuckova ivanahuckova added add to changelog and removed no-changelog Skip including change in changelog/release notes labels Apr 25, 2023
@ivanahuckova ivanahuckova changed the title Logs Context: Always display context toggle Loki: Always display log context toggle Apr 25, 2023
@zerok zerok modified the milestones: 10.0.0, 10.0.0-preview May 31, 2023
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.

Loki: Log context is not offered with non-equal line filters
6 participants