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

[Feature Anywhere] Add view events flyout #3415

Merged

Conversation

ohltyler
Copy link
Member

@ohltyler ohltyler commented Feb 10, 2023

Signed-off-by: Tyler Ohlsen ohltyler@amazon.com

Currently this PR has many dependent ones that will need to be merged first. This includes:

These all involve finalizing the rest of the chart render workflow, and the VisLayer design.

When these have been merged, we can finish the following items for this flyout:


What can be reviewed now

  • all files under view_events_flyout/components/
  • all files under vis_type_vega plugin
  • all files under vis_type_vislib plugin

Description

This PR adds the 'View events' flyout which will open via an event click on a chart with VisLayers enabled, or in the vis options menu via View events button. It shows the base vis with the event annotations, and below it a breakdown of each set of events produced by each plugin resource. It is categorized by plugin resource type as well.

Technical details, more information, and example mocks can be found in the related issue #3155

Validation

To test the feature:

  • clone locally (no extra plugins needed)
  • create an eligible line visualization (for eligibility see Finalize eligibility check for augmenting visualizations by vis augmenter #3687 until docs are finished)
  • embed the visualization on some dashboard
  • update the timestamps in the list of test VisLayers to be within the time range of your visualization
  • events should appear, and can be clicked on the chart directly or via "view events" button in the options dropdown for the embeddable panel, as seen in sample video below

Implementation details

Components

ViewEventsFlyout - the base component where all embeddables are created from the source vis embeddable. Persists the event visualizations by type via a map mapping resource type -> list of embeddables

DateRangeItem - contains the selected date range and refresh button. Hooks into the reload() function defined in ViewEventsFlyout to reload all of the embeddables when clicked.

BaseVisItem - the re-created base vis embeddable containing all of the events

EventsPanel - the base component to show all of the event annotation charts. Contains a set of PluginEventsPanels

PluginEventsPanel - contains a set of EventVisItems for a particular plugin resource type

EventVisItem - contains an event vis embeddable, including the resource name, url, and event count

EventVisItemIcon - contains the icon with the event count or the error icon with error message details, if applicable.

TimelinePanel - contains the empty embeddable. Used to re-create the timeline and is a static component on the bottom of the flyout

All of the embeddables are using the EmbeddablePanel component, which takes care of individual component lifecycles for us, including the rendering, refreshing, and destroying. This is fetched from the embeddable service function's getEmbeddablePanel().

The VisAugmenterEmbeddableConfig

The flyout requires rendering essentially the same visualization, but with various visual changes and certain data filtered out; for example, showing only data from one VisLayer, showing just the x-axis, hiding the y axes, adding horizontal padding, etc. To account for these changes, we need to configure this at the top-level such that it is taken into account when generating the high-level vega-lite spec at the visualize_embeddable level, all the way down to the compiled vega spec and instantiated vega view. To do this, we add the VisAugmenterEmbeddableConfig. This is a high-level interface (independent of charting libraries/details) that is used to specify these necessary configurations:

  • visLayerResourceIds - the list of VisLayers with particular plugin resource IDs we want to show on the chart
  • inFlyout - whether we are rendering this embeddable within the view events flyout or not
  • flyoutContext - whether we are rendering a base vis (the original vis), event vis (the individual VisLayer charts), or timeline vis (the bottom vis only showing the x-axis / timeline)
  • leftValueAxisPadding - whether we need to add padding to the left of the value axis for alignment
  • rightValueAxisPadding - whether we need to add padding to the right of the value axis for alignment

Demo video:
demo-new.webm

Issues Resolved

Closes #3155
Closes #3580
Closes #3485

Check List

  • All tests pass
    • yarn test:jest
    • yarn test:jest_integration
    • yarn test:ftr
  • New functionality includes testing.
  • New functionality has been documented.
  • Update CHANGELOG.md
  • Commits are signed per the DCO using --signoff

@ohltyler
Copy link
Member Author

ohltyler commented Mar 9, 2023

Details on error handling can be seen in this issue: #3580. We can include all of those details as part of the work for this PR since it will be needed to finish all rendering scenarios of the view events page.

@ohltyler ohltyler force-pushed the view-events-2 branch 4 times, most recently from 777038f to 10dfac3 Compare March 13, 2023 22:32
@ohltyler
Copy link
Member Author

While the final items (custom chart functionality and error handling) are currently blocked, the bulk of this PR is ready and able to be reviewed. Please read PR description and corresponding for some more context, before starting review.

@codecov-commenter
Copy link

codecov-commenter commented Mar 31, 2023

Codecov Report

Merging #3415 (c33fcb5) into feature/feature-anywhere (ec4c1df) will decrease coverage by 0.02%.
The diff coverage is 45.87%.

@@                     Coverage Diff                      @@
##           feature/feature-anywhere    #3415      +/-   ##
============================================================
- Coverage                     66.44%   66.42%   -0.02%     
============================================================
  Files                          3247     3263      +16     
  Lines                         62553    62812     +259     
  Branches                       9659     9708      +49     
============================================================
+ Hits                          41563    41724     +161     
- Misses                        18676    18775      +99     
+ Partials                       2314     2313       -1     
Flag Coverage Δ
Linux 66.36% <45.87%> (-0.08%) ⬇️
Windows 66.36% <45.87%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...s/embeddable/public/lib/panel/embeddable_panel.tsx 64.13% <ø> (ø)
src/plugins/embeddable/public/plugin.tsx 76.53% <ø> (ø)
src/plugins/vis_augmenter/public/index.ts 0.00% <ø> (ø)
src/plugins/vis_augmenter/public/plugin.ts 0.00% <0.00%> (ø)
src/plugins/vis_augmenter/public/test_constants.ts 100.00% <ø> (ø)
src/plugins/vis_augmenter/public/types.ts 100.00% <ø> (ø)
...lic/view_events_flyout/components/events_panel.tsx 0.00% <0.00%> (ø)
...w_events_flyout/components/plugin_events_panel.tsx 0.00% <0.00%> (ø)
src/plugins/vis_type_vega/public/vega_type.ts 44.44% <0.00%> (ø)
...lugins/vis_type_vega/public/vega_view/vega_view.js 0.00% <0.00%> (ø)
... and 27 more

... and 10 files with indirect coverage changes

@aoguan1990
Copy link
Contributor

@lezzago Can you please provide the preliminary review on this issue? Thanks!

@joshuarrrr
Copy link
Member

@ohltyler I'll go ahead and review this, but looks like there are a few conflicts that will need to be resolved.

@ohltyler
Copy link
Member Author

@ohltyler I'll go ahead and review this, but looks like there are a few conflicts that will need to be resolved.

Yes, I've had to prematurely pull some uncommitted changes from other PRs to ensure integration was working as expected, hence the weird conflicts. I will clean these up once the final dependent PR #4120 is merged.

Copy link
Member

@joshuarrrr joshuarrrr left a comment

Choose a reason for hiding this comment

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

A couple of minor questions to assist my understanding. I've gotten through changes to existing plugins - there's a lot of changes to vis_augmenter I haven't reviewed yet, but those are less critical because it's a brand new component that's much less likely to introduce regressions. I'll finish a quick review of those files in the morning.

@ohltyler
Copy link
Member Author

@joshuarrrr rebased without making any changes. Follow-up commits will be to address comments.

Signed-off-by: Tyler Ohlsen <ohltyler@amazon.com>
Copy link
Member

@joshuarrrr joshuarrrr left a comment

Choose a reason for hiding this comment

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

Overall this looks great and was clearly a ton of work. I mostly have some minor questions and comments, but no blockers.

Signed-off-by: Tyler Ohlsen <ohltyler@amazon.com>
Signed-off-by: Tyler Ohlsen <ohltyler@amazon.com>
Copy link
Member

@joshuarrrr joshuarrrr left a comment

Choose a reason for hiding this comment

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

Thanks @ohltyler for all the fixes. I'm going to mark as approved, but I'll need to reapprove after we update the snapshots.

Signed-off-by: Tyler Ohlsen <ohltyler@amazon.com>
@joshuarrrr
Copy link
Member

@ohltyler There are still some flyout unit test failures - maybe something got messed up in the revision commits: https://github.com/opensearch-project/OpenSearch-Dashboards/actions/runs/5085933677/jobs/9139910721?pr=3415

Signed-off-by: Tyler Ohlsen <ohltyler@amazon.com>
Signed-off-by: Tyler Ohlsen <ohltyler@amazon.com>
@ohltyler
Copy link
Member Author

@manasvinibs We are still looking to get a review from @ashwin-pc who has more context on the PR - let's hold off on merging until that is done

@ohltyler
Copy link
Member Author

Discussed with @ashwin-pc - will look into the possibility of removing the dependency on the embeddables plugin as well as the added config to VisualizeEmbeddable, and instead render the charts using ReactExpressionRenderer, similar to what VisBuilder plugin does. This can simplify the logic needed to produce such charts, and allow us to control the input config that we've needed to add to VisualizeEmbeddable (the VisAugmenterEmbeddableConfig).

@ohltyler
Copy link
Member Author

Will follow up in a separate PR for this.

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.

8 participants