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 UI] view log line in context #58487

Closed
3 of 7 tasks
katrin-freihofner opened this issue Feb 25, 2020 · 25 comments · Fixed by #62198
Closed
3 of 7 tasks

[Logs UI] view log line in context #58487

katrin-freihofner opened this issue Feb 25, 2020 · 25 comments · Fixed by #62198
Assignees
Labels
Feature:Logs UI Logs UI feature Team:Infra Monitoring UI - DEPRECATED DEPRECATED - Label for the Infra Monitoring UI team. Use Team:obs-ux-infra_services v7.8.0

Comments

@katrin-freihofner
Copy link
Contributor

katrin-freihofner commented Feb 25, 2020

We defined a way for the Logs UI to allow users to quickly see a log message in context.

Find more background information and related discussions in the design issues
#40808, #50304

1. Select

When selecting a logline, a (popup) menu should appear that lets users select the Logline in context action.
stream

Additionally, the popup menu should show a link to view the details of a log message.

2. Context

The applied context is defined by host.name:[host.name] and log.file.path:[file.path] or [container.id].
view in context

The lookup hierarchy for these fields should be as follows:

  1. Use container.id if that value exists
  2. Use host.name + log.file.path if both values exist
  3. If neither of these two criteria are met, don't show the option in the menu The option will always be visible, but disabled if we don't have the data (Disable vs hide "View log line in context" feature in actions menu logs#13)

3. Highlight

The selected log line stays within the view and is highlighted as a reference.


Design work

Design issue
Figma prototype


Changes in the APIs

export const logEntryRT = rt.type({
  id: rt.string,
  cursor: logEntriesCursorRT,
  columns: rt.array(logColumnRT),
  context: rt.partial({
    'container.id': rt.string,
    'host.name': rt.string,
    'log.file.path': rt.string,
  })
});

The API will return whatever fields are available. The UI will then make the decision of which fields to use for the context.

Changes in the UI

interface Props {
  // If we go for a `-Infinity` to `Infinity` range this is not necessary
  startTimestamp: number;
  endTimestamp: number;
}

interface State {
  centerEntry: LogEntry | null;
  entries: LogEntry[];
  isVisible: boolean;
}

interface Callbacks {
  showContextModal: (entry: LogEntry) => void;
  hideContextModal: () => void;

  // If we don't do pagination this is not necessary
  fetchPreviousPage: () => void;
  fetchNextPage: () => void;
}

const useViewLogInContext(props: Props): [State, Callbacks] {
  // use `props.centerEntry.context` to fetch the entries around the given line, with the context that makes sense.
}
  • Place the Provider for the hook
  • Create the modal component
  • Add an entry to the context menu to show/hide the modal component
@lizozom lizozom added the Feature:Logs UI Logs UI feature label Feb 27, 2020
@sgrodzicki sgrodzicki added the Team:Infra Monitoring UI - DEPRECATED DEPRECATED - Label for the Infra Monitoring UI team. Use Team:obs-ux-infra_services label Mar 3, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/logs-metrics-ui (Team:logs-metrics-ui)

@jasonrhodes
Copy link
Member

@roncohen what was your suggested hierarchy of fields for the context lookup?

A

  1. host.name + log.file.path if both values exist
  2. container.id if that value exists
  3. don't show option

B

  1. container.id if that value exists
  2. host.name + log.file.path if both values exist
  3. don't show option

C
something else?

@jasonrhodes
Copy link
Member

@katrin-freihofner thank you for all the work on this!!! Now that we're aligned on a direction for the first iteration, what do you and @mukeshelastic think about softening some of the language in the context pop up? Instead of showing the host/path values as applied filters, maybe something like "Now viewing logs from [/var/log/a...b.log] on host [filebeat-2]", or "Now viewing logs from container [whatever-id]"?

@katrin-freihofner
Copy link
Contributor Author

@katrin-freihofner thank you for all the work on this!!! Now that we're aligned on a direction for the first iteration, what do you and @mukeshelastic think about softening some of the language in the context pop up? Instead of showing the host/path values as applied filters, maybe something like "Now viewing logs from [/var/log/a...b.log] on host [filebeat-2]", or "Now viewing logs from container [whatever-id]"?

I like that - I will give it a try. @mukeshelastic what is your opinion?

@roncohen
Copy link
Contributor

roncohen commented Mar 3, 2020

@jasonrhodes lets go for B as a start

btw. i filed this: elastic/ecs#770

@jasonrhodes
Copy link
Member

@roncohen thanks, I updated the ticket description with option B.

@katrin-freihofner
Copy link
Contributor Author

@katrin-freihofner thank you for all the work on this!!! Now that we're aligned on a direction for the first iteration, what do you and @mukeshelastic think about softening some of the language in the context pop up? Instead of showing the host/path values as applied filters, maybe something like "Now viewing logs from [/var/log/a...b.log] on host [filebeat-2]", or "Now viewing logs from container [whatever-id]"?

Like this:

Screenshot 2020-03-03 at 17 31 48

@roncohen
Copy link
Contributor

roncohen commented Mar 3, 2020

personally, I prefer the previous design that shows the filter in square boxes as opposed to the more verbose ones here. @jasonrhodes is your concern that people will not understand the filter without the more text-full description?

@jasonrhodes
Copy link
Member

@roncohen I actually think they look more like applied filters that can be modified and interacted with, rather than just being information about the fixed context that you're looking at. But I don't feel super strongly about it, I think they both work.

@mukeshelastic
Copy link

Sorry for the delay in responding to this thread. Katrin and I discussed and shared the same concern that Jason mentioned above. Showing it as filter implies that it is filter bar ( similar to stream filter bar exp) and that isn't our intention here.

@roncohen
Copy link
Contributor

roncohen commented Mar 6, 2020

OK, i don't think we need to add text to convey that these are static filters. There are lots of places we have label/value style information shown without text. How about <key>: value, key: <value> on a plain white background?

@mukeshelastic
Copy link

As long as we can convey that these are non-editable filters, it should work. If we follow the pattern you suggested in other place to convey the non-edit-ability, then makes sense to follow it here as well.

@afgomez
Copy link
Contributor

afgomez commented Mar 20, 2020

I'm looking into the work necessary for this task. I have two questions:

Since we have now merged the SuperDatePicker and everything is fenced by a date range, I wonder if we should apply the date range in this view, or if we should consider all the data we have for the context.

So, if the user selects a range of now-15m to now, should the View Log in Context UI show lines only in that range or should it not limit itself?

The second question is: should this view have infinite scroll? How much context is enough context? If we think this is a "nice to have" we can do it as a separate task to get the feature merged earlier.

@afgomez
Copy link
Contributor

afgomez commented Mar 20, 2020

Here's a list of what I think needs to be done. Many of these changes can be done in separate PRs.

Changes in the APIs

  • Add a context property to the logEntryRT type.
export const logEntryRT = rt.type({
  id: rt.string,
  cursor: logEntriesCursorRT,
  columns: rt.array(logColumnRT),
  context: rt.partial({
    'container.id': rt.string,
    'host.name': rt.string,
    'log.file.path': rt.string,
  })
});

The API will return whatever fields are available. The UI will then make the decision of which fields to use for the context.

Changes in the UI

  • Adjust colors in the highlighted row lines to match the new design.
  • Replace "expand" icon with contextual menu.
  • Create useViewLogInContext hook
interface Props {
  centerEntry: LogEntry | null;
  // If we go for a `-Infinity` to `Infinity` range this is not necessary
  startTimestamp: number;
  endTimestamp: number;
}

interface State {
  centerEntry: LogEntry | null;
  entries: LogEntry[];
  isVisible: boolean;
}

interface Callbacks {
  showModal: () => void;
  hideModal: () => void;

  // If we don't do pagination this is not necessary
  fetchPreviousPage: () => void;
  fetchNextPage: () => void;
}

const useViewLogInContext(props: Props): [State, Callbacks] {
  // use `props.centerEntry.context` to fetch the entries around the given line, with the context that makes sense.
}
  • Place the Provider for the hook
  • Create the modal component
  • Add an entry to the context menu to show/hide the modal component

@weltenwort
Copy link
Member

It's great to see that plan up front. Looks sensible to me with only a few questions coming to mind:

Place the Provider for the hook

We might not even need a provider. Unless the prop drilling becomes too burdensome the hook might just live locally in the modal component.

Add a context property to the logEntryRT type.

Thinking out loud: I wonder if the context fields should be hard-coded in the API or whether the client should include a list of context fields in the request that it is prepared to handle. That would make it more extensible in the future, but also more complicated now. So hard-coding it might be appropriate for now. 💭

Do you already have an idea about how to split it up into PRs?

@afgomez
Copy link
Contributor

afgomez commented Mar 20, 2020

Do you already have an idea about how to split it up into PRs?

One for the API changes, one for the visible changes in the stream (color + menu, initially with only one item), and a bigger one for the hook + modal.

The first two can be done and reviewed in parallel. The last one builds on top of them.

@weltenwort
Copy link
Member

Sounds reasonable 👍 Depending on the answers to your questions about the context size and scrolling above a static size might be good enough to merge initially with refinements being added in follow-ups.

A few more thoughts about the hook arguments, state and return values that you outlined:

  • The centerEntry should probably either be an argument or state, but not both. Intuitively I would say it is externally tracked state (e.g. in the url using
    export const useUrlState = <State>({
    ). A small wrapper hook could then expose showContextModal(centerEntry: Cursor) and hideContextModal.
  • Is an explicit isVisible flag needed? Couldn't centerEntry === null be used to hide the modal? That would make a state where the overlay is visible but no center is set unrepresentable.

@afgomez
Copy link
Contributor

afgomez commented Mar 20, 2020

Yeah, good points. I would then keep it only in the state. The center point needs to be set from outside (from the menu). We could set that with the callback.

I'm going to start doing the backend bit :)

@roncohen
Copy link
Contributor

I caught up with @mukeshelastic and we're aligned on the following:

Since we have now merged the SuperDatePicker and everything is fenced by a date range, I wonder if we should apply the date range in this view, or if we should consider all the data we have for the context.
So, if the user selects a range of now-15m to now, should the View Log in Context UI show lines only in that range or should it not limit itself?

Let’s disregard the time picker when clicking “View in context”. If the user has selected a very narrow window in order to find that specific error log, and then clicks “View in context” it’s not useful to limit it to the time window they have selected. We have a limit on the number of lines above and below and we should “just” query enough time range to fill it out.

The second question is: should this view have infinite scroll? How much context is enough context? If we think this is a "nice to have" we can do it as a separate task to get the feature merged earlier.

yes, infinite scroll can definitely wait.

in short, we should aim to show the user a specific number of lines before and after the selected line, disregarding the time window selected

@weltenwort
Copy link
Member

Querying ES without a date range filter can seriously impact the cluster. That was one of the main reasons for introducing the date range picker. Previously I had implemented an exponential widening of the interval until the desired number of entries had been found to work around this. @afgomez that functionality has disappeared, right?

@mukeshelastic
Copy link

@weltenwort If I understand your concern correctly, If we hard code the number of log lines to be shown as total of 1000 and if the logs are sparsely distributed over a large time range for the applied filter of index, filepath and hostname then it will have significant performance impact on the cluster?

@weltenwort
Copy link
Member

weltenwort commented Mar 30, 2020

Yes, exactly. If we don't specify a date range Elasticsearch will have to query all shards. With a date range it will rewrite the query to ignore most shards for the recommended date- or size-based index life cycle setup.

When I initially wrote the context view feature (without a date filter) for discover I got feedback from large-scale users that the queries stressed their clusters beyond reason (see #15143 for one such report).

@mukeshelastic
Copy link

Thanks for the confirmation @weltenwort.

@afgomez My suggestion would be to apply date filter with value set from date picker.
Does this additional scope of adding date time filter put this feature at 7.8 release risk?

For the infinite scroll, let's implement it in another issue. In this issue, let's restrict the scope to:

  1. apply date time filter
  2. After applying filter, restrict the number of log lines below a fixed number. Say total lines as 5000 with 2500 above and below? I don't know what would make a good number so this is a pure assumption.

Thoughts? @roncohen @weltenwort @afgomez

@mukeshelastic
Copy link

Created a separate issue for infinite scrolling #61848

@afgomez
Copy link
Contributor

afgomez commented Mar 31, 2020

My concern is that if the date range specified is too narrow, the "view log in context" is not going to show a lot of context and make the feature less useful.

But maybe users will never run into that problem 🤷‍♂️

To move forward, I will use the existing date range, and if we see it is a problem we can add a minimum date range before and after the line.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Logs UI Logs UI feature Team:Infra Monitoring UI - DEPRECATED DEPRECATED - Label for the Infra Monitoring UI team. Use Team:obs-ux-infra_services v7.8.0
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants