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

Add ApexChart based visualizations #3040

Open
wants to merge 50 commits into
base: master
Choose a base branch
from
Open

Conversation

sydp
Copy link
Collaborator

@sydp sydp commented Feb 18, 2024

Adds ApexChart based visualizations to frontend-ng

Checks

  • All tests succeed.
  • Unit tests added.
  • e2e tests added.
  • Documentation updated.

Closing issues

Put closes #XXXX in your comment to auto-close the issue that your PR fixes
(if such).

cast.webm

@sydp sydp marked this pull request as ready for review February 18, 2024 23:46
@berggren berggren self-requested a review February 22, 2024 07:49
@berggren
Copy link
Contributor

Hey @sydp This is a big PR :) It will take some time to review. I'll catch up with you offline to see if we can break it up a bit.

@sydp sydp requested a review from jkppr February 26, 2024 08:39
@jkppr jkppr self-assigned this Apr 4, 2024
@rocketeeer
Copy link
Contributor

bump

Copy link
Collaborator

@jkppr jkppr left a comment

Choose a reason for hiding this comment

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

Alright, given the size of the PR I have to split the review work into multiple sessions. So this is part 1/X. I'll cover the files in the Visualization folder in part 2.

Copy link
Collaborator

@jkppr jkppr left a comment

Choose a reason for hiding this comment

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

(2/X) I continued with some of the chart files. Please take a look at the comments below.

timesketch/frontend-ng/src/views/Visualization.vue Outdated Show resolved Hide resolved
Copy link
Collaborator

Choose a reason for hiding this comment

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

The minimum settings we need to render an aggregation we need field, aggregation type and then some information depending on the type (e.g. max values and chart type).
Would it be a lot of effort to trigger & auto-update the preview on the right side when the minimum information are provided or changed? (Instead of the user clicking the reload button)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I am somewhat reluctant to auto update since this will trigger an aggregation on the server end which could be expensive/slow, depending on the selected field/aggregation type (and users may unintentionally select wrong values)

Not sure what the effort, I don't imagine it would be "too much" (famous last words) but I'll hold off for now until you confirm this is what you would like to happen.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks, I get your concerns with auto-update. The current behaviour is some mix: If I generate and change the chart, it will reload automatically. I guess this is due to changing the chart just being in the frontend and not re-running the aggregation on the backend?

Let's go with a load/reload button for now and keep the behaviour the same. So no auto change for switching charts for example. We can move to auto-update later when we have some metrics on how people are using the feature.

What do you think?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I guess this is due to changing the chart just being in the frontend and not re-running the aggregation on the backend?

Yes, that's correct - the Load button is only for retrieving the aggregated results so the user can preview them in the selected chart (and update in the front-end) before deciding to save/clear/cancel.

I'm fine with leaving as keeping the behaviour as is, but of course equally happy to tweak if desired.

Copy link
Collaborator

@jkppr jkppr left a comment

Choose a reason for hiding this comment

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

3/X

Copy link
Collaborator

Choose a reason for hiding this comment

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

The BarChart has some truncation for the field names. But the Column chart and LineChart don't do the truncation. See screenshots:
image

vs

image

Is this something that can be fixed easily or something for a future PR?

Copy link
Collaborator Author

@sydp sydp May 23, 2024

Choose a reason for hiding this comment

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

It might be difficult to control when labels can be unexpectedly large. Not an ideal "fix" but the user can simply turn off x-axis labels and hover over bar/data point to see the value (similar to the histogram chart in the explore view).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

For info, details on the behaviour of labels from ApexCharts:

https://apexcharts.com/docs/multiline-text-and-line-breaks-in-axes-labels/

Comment on lines +30 to +37
<v-autocomplete
outlined
v-model="selectedAggregator"
:disabled="disableAggregator"
:items="aggregators"
label="Aggregate events by"
:rules="[rules.required]"
></v-autocomplete>
Copy link
Collaborator

Choose a reason for hiding this comment

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

I ran some small user-tests with colleagues in the office and one common feedback was that it is unclear what the aggregation options mean and how they work.
What do you think about using a combination of title and subtitle for each entry? Where the subtitle explains what this aggregation actually does. Similar to this mock for the DFIQ implementation:
image

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

by aggregation options, do you mean the aggregators themselves or the options for each aggregator?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry, I'm talking about the meaning of "Auto Time Interval", "Rare terms", "Time interval" and "Top K terms".

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@sydp
Copy link
Collaborator Author

sydp commented May 23, 2024

#3040 (comment)

I'm not able to comment to this in thread but this seems to be working as intended on my machine. perhaps it could be stale data after changing the max document count value. does clicking the "load button" produce the expected data?

edit: disregard, I think i am able to replicate. seems the value isn't propagating.

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.

None yet

4 participants