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

Logs: Add permalink to log lines #69464

Merged
merged 29 commits into from Jun 16, 2023
Merged

Logs: Add permalink to log lines #69464

merged 29 commits into from Jun 16, 2023

Conversation

svennergr
Copy link
Contributor

@svennergr svennergr commented Jun 2, 2023

What is this feature?

This feature add the log line permalinking functionality to Grafana. Users in Explore can use a button to generate a shortlink pointing to one exact logline.

Who is this feature for?

Grafana Logs users.

Which issue(s) does this PR fix?:

Fixes #67379

Special notes for your reviewer:

Currently this works only without the exploreScrollableLogsContainer feature toggle. I'll work on a fix for that in a separate PR.

  1. Start Grafana
  2. Start a source of logs, e.g. make devenv sources=loki
  3. Go to Explore and query logs
  4. Hover over a logline
  5. Click the "Link" button
  6. A short url is copied to the clipboard

@github-actions
Copy link
Contributor

github-actions bot commented Jun 2, 2023

Backend code coverage report for PR #69464
No changes

@github-actions
Copy link
Contributor

github-actions bot commented Jun 2, 2023

Frontend code coverage report for PR #69464

Plugin Main PR Difference
explore 87.09% 87.02% -.07%

@svennergr
Copy link
Contributor Author

One thing that I noticed and I am unsure about the correct solution:

Screen.Recording.2023-06-02.at.15.59.58.mov

Basically, if a user is coming from a permalink the UI scrolls to that logline. That's fine. Let's say users then run different queries with results not containing the logline, that's also fine. But then users run a query with results containing the permalinked logline, the visualisation will now also scroll to that logline.

I kind of like this, but it can also be super distracting if people aren't interested in the linked logline anymore.
I was thinking about adding a second state to the permalink-button, which stays "active" so users can toggle off the "linked" state.

@matyax
Copy link
Contributor

matyax commented Jun 2, 2023

@svennergr Regarding your last comment, I think after using the permalink and getting to the linked line, the "permalinked" state should be gone, i.e., even if they run the same query again, they should not be scrolled again. The only way to return to the linked log line should be by browsing the link again.

That is how, IMO, links usually work, and feels like the expected UX for it.

@svennergr
Copy link
Contributor Author

the "permalinked" state should be gone

Just to clarify, you wouldn't remove highlighting from the logline, would you?

@matyax
Copy link
Contributor

matyax commented Jun 2, 2023

I would. I don't think I would expect to see the line highlighted again after changing the query or re-executing it.

Another thing that stands out to me is that I think we should highlight the permalinked line a little bit more, because as it currently is it can be easily missed. One big reason for this is accessibility, where slight contrast changes might not be noticeable for some people.

@svennergr
Copy link
Contributor Author

svennergr commented Jun 2, 2023

Yea, I talked about that with @niat22 already. It won't be part of this PR though.

Thanks for your feedback - much appreciated!

@svennergr svennergr force-pushed the svennergr/logs-permalink branch 3 times, most recently from 54eb0fc to 68ec5cf Compare June 13, 2023 10:56
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.

THIS IS SO GOOD 🔥! Left some feedback/questions bellow.

.betterer.results Outdated Show resolved Hide resolved
public/app/features/explore/Logs/Logs.test.tsx Outdated Show resolved Hide resolved
public/app/features/explore/Logs/Logs.test.tsx Outdated Show resolved Hide resolved
public/app/features/logs/components/LogRowMessage.tsx Outdated Show resolved Hide resolved
public/app/features/logs/components/LogRowMessage.test.tsx Outdated Show resolved Hide resolved
public/app/features/logs/components/LogRowMessage.test.tsx Outdated Show resolved Hide resolved
public/app/features/logs/components/LogRow.test.tsx Outdated Show resolved Hide resolved
public/app/features/logs/components/LogRow.test.tsx Outdated Show resolved Hide resolved
public/app/features/explore/Logs/Logs.tsx Show resolved Hide resolved
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.

Left couple more feedback mostly related to details, nothing big

public/app/features/logs/components/LogRowMessage.tsx Outdated Show resolved Hide resolved
public/app/features/logs/components/LogRowMessage.tsx Outdated Show resolved Hide resolved
public/app/features/logs/components/LogRowMessage.tsx Outdated Show resolved Hide resolved
public/app/features/logs/components/getLogRowStyles.ts Outdated Show resolved Hide resolved
public/app/features/explore/Logs/Logs.tsx Show resolved Hide resolved
public/app/features/explore/Logs/Logs.tsx Show resolved Hide resolved
svennergr and others added 3 commits June 16, 2023 13:39
Co-authored-by: Ivana Huckova <30407135+ivanahuckova@users.noreply.github.com>
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.

LGTM

@svennergr svennergr merged commit 6863705 into main Jun 16, 2023
17 checks passed
@svennergr svennergr deleted the svennergr/logs-permalink branch June 16, 2023 12:07
harisrozajac pushed a commit that referenced this pull request Jun 30, 2023
* create explore panel state for logs

* add props to LogRows and unify

* pass properties from explore to logs

* add css

* implement button and scrolling

* export and use `getUrlStateFromPaneState`

* make `scrollIntoView` optional

* change state handling for permalinks

* change link icon

* removed unused state

* add tests for `LogRowMessage`

* remove unused prop

* fix name

* reorg component

* add `LogRow` tests

* add test for `Logs`

* Update public/app/features/logs/components/LogRow.test.tsx

Co-authored-by: Ivana Huckova <30407135+ivanahuckova@users.noreply.github.com>

* Update public/app/features/explore/Logs/Logs.test.tsx

Co-authored-by: Ivana Huckova <30407135+ivanahuckova@users.noreply.github.com>

* improve types in test

* fix props export in Logs.tsx

* fix props export in LogRowMessage.tsx

* fix props export in LogRow.tsx

* fixed import

* fix theme import

* remove hidden style

* add better test names

* change to `log line` rather logline

Co-authored-by: Ivana Huckova <30407135+ivanahuckova@users.noreply.github.com>

* fix tooltips

* remove unused css

---------

Co-authored-by: Ivana Huckova <30407135+ivanahuckova@users.noreply.github.com>
@ricky-undeadcoders ricky-undeadcoders modified the milestones: 10.1.x, 10.1.0 Aug 1, 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.

Logs: Permalink implementation
5 participants