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: Display log lines context #17097

Merged
merged 22 commits into from
May 20, 2019
Merged

Explore: Display log lines context #17097

merged 22 commits into from
May 20, 2019

Conversation

dprokop
Copy link
Member

@dprokop dprokop commented May 15, 2019

#15140

This PR introduces a possibility to display Loki log line context, meaning that it when clicked on "Show context" in log row, additional log rows for a given row's log stream will be fetched and diaspayed.

react-use

In this PR react-use lib is introduced for common react hooks to be available in Grafana's codebase. Main reason for that is usage of useAsync in LogRowContextProvider to make fetching context and additional context rows easy. Instead of using React's class component lifecycle, hooks take responsibility for managing the context state and delivering it to log context components.

TODO:

  • Implement fetching additional context
  • Fix ClickOutsideWrapper
  • Improve UI

@dprokop dprokop requested a review from davkal May 15, 2019 15:04
@dprokop dprokop added this to the 6.3 milestone May 15, 2019
@dprokop dprokop self-assigned this May 15, 2019
@davkal
Copy link
Contributor

davkal commented May 15, 2019

Looks very promising 🚀

Here is some feedback on what's already there:

  • the area of the context lines looks too similar to the rest, I think it needs more visual differentiation
  • the after context included the pivot line
  • the content of the after context keeps changing order, see the times in this screenshot:

Screenshot 2019-05-15 at 17 13 44

Otherwise great stuff!

package.json Show resolved Hide resolved
@dprokop

This comment has been minimized.

@davkal
Copy link
Contributor

davkal commented May 16, 2019

Looking really good now. More feedback:

  • the outside click to close works well
  • the pivot line still gets repeated in the after context (see pic)
  • i'd add a mini border radius to the context containers
  • the after context container shadow overlaps with the pivot row (see pic)
  • i'd differentiate the "Showing X rows" from "Load Y more" a bit more, e.g., make the status pale, and keep the action vibrant.

Screenshot 2019-05-16 at 15 03 36

Stretch goals:

  • loading more should only load more for the given direction
  • add a mini search bar next to Showing and Load, to quickly filter lines by a term

This feature is really shaping up 🚀

@dprokop

This comment has been minimized.

@dprokop dprokop marked this pull request as ready for review May 17, 2019 10:08
@davkal
Copy link
Contributor

davkal commented May 17, 2019

Last UX item: Some context queries return with an error (or no results). In my case it was an error 502, but there was nothing shown. Would be great to get 0-results and error feedback.
Notice how there are no context containers:
Screenshot 2019-05-17 at 12 19 29

I think that's it for this PR.

Follow-up work:

  1. Small search field in the context containers to quickly filter for a keyword

  2. Been playing with this now a bit and one thing for highly aggregated log streams (streams of lots of sub stream), the context query results can be really minimal, say 1-4 lines. In that case it'd be great if I could A) see the label set used for the context query, and B) remove individual labels so that fewer streams are filtered out.

Copy link
Contributor

@davkal davkal left a comment

Choose a reason for hiding this comment

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

Works really well. Minor nits here and there, but otherwise good to merge.

public/app/features/explore/LogRow.tsx Outdated Show resolved Hide resolved
public/app/features/explore/LogRow.tsx Outdated Show resolved Hide resolved
public/app/features/explore/LogRow.tsx Show resolved Hide resolved
public/app/features/explore/LogRow.tsx Outdated Show resolved Hide resolved
public/app/features/explore/LogRowContext.tsx Show resolved Hide resolved
public/app/features/explore/state/actionTypes.ts Outdated Show resolved Hide resolved
@dprokop dprokop merged commit 12e0616 into master May 20, 2019
/**
* Retrieve context for a given log row
*/
getLogRowContext?(row: any, limit?: number): Promise<DataQueryResponse>;
Copy link
Member

Choose a reason for hiding this comment

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

is a row completely untyped? don't we need the LogsModel and the row model is LogRowModel (and could be good to have both).

Copy link
Member Author

@dprokop dprokop May 20, 2019

Choose a reason for hiding this comment

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

Agree about LogRowModel. Not necessarily abut LogsModel - why do you think it could be helpful?

Copy link
Member

@torkelo torkelo May 20, 2019

Choose a reason for hiding this comment

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

I am not 100% sure a single row can be enough information to construct a query for the context. You might need labels and the original query & response to create the context query.

@@ -282,6 +287,10 @@ export interface DataQueryResponse {
data: DataQueryResponseData[];
}

export interface LogRowContextQueryResponse {
data: Array<Array<string | DataQueryError>>;
Copy link
Member

Choose a reason for hiding this comment

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

this response type looks very strange, should this not be SeriesData?

@dprokop dprokop deleted the explore/logs-context branch May 20, 2019 07:02
@marefr marefr changed the title Explore: display log line context Explore: Display log lines context May 20, 2019
markelog added a commit that referenced this pull request May 20, 2019
* 'master' of github.com:grafana/grafana:
  LDAP: add tests for initialBind (#17132)
  Explore: Adds Live option for supported datasources (#17062)
  alerting: fix a bunch of lint issues. (#17128)
  chore: mocks plugin loader for DataSourceSettingsPage tests (#17157)
  Release: Improved cherry pick task (#17087)
  Explore: Fix selection/copy of log lines (#17121)
  Explore: Fix empty space in toolbar on smaller devices (#17110)
  Explore: display log line context  (#17097)
  Plugins: expose rxjs matching 6.4.0 (#17148)
  Chore: fix codespell issue with build (#17144)
@yandingkang
Copy link

@dprokop Is it possible to display more than 10 log rows when I click "Load 10 more" ?

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.

5 participants