[Feature anywhere] Add annotation click handler #3777

amsiglan · 2023-04-04T07:21:32Z

Description

This PR adds framework to register event handers with vega based visualizations by adding config and providing handler in the vis_augmenter plugin.

Issues Resolved

#3317

Check List

Signed-off-by: Amardeepsingh Siglani <amardeep7194@gmail.com>

joshuarrrr · 2023-04-05T00:29:55Z

@amsiglan There are some unit test failures to check and fix: https://github.com/opensearch-project/OpenSearch-Dashboards/actions/runs/4605088099/jobs/8136698336?pr=3777

Signed-off-by: Amardeepsingh Siglani <amardeep7194@gmail.com>

codecov-commenter · 2023-04-05T23:24:57Z

Codecov Report

Merging #3777 (6d9b9b4) into feature/feature-anywhere (609bcc1) will decrease coverage by 0.02%.
The diff coverage is 15.15%.

@@                     Coverage Diff                      @@
##           feature/feature-anywhere    #3777      +/-   ##
============================================================
- Coverage                     66.52%   66.50%   -0.02%     
============================================================
  Files                          3244     3246       +2     
  Lines                         62448    62508      +60     
  Branches                       9642     9654      +12     
============================================================
+ Hits                          41541    41574      +33     
- Misses                        18600    18626      +26     
- Partials                       2307     2308       +1

Flag	Coverage Δ
Linux	`66.45% <15.15%> (-0.01%)`	⬇️
Windows	`66.45% <15.15%> (-0.02%)`	⬇️

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

Impacted Files	Coverage Δ
src/plugins/ui_actions/public/index.ts	`100.00% <ø> (ø)`
src/plugins/vis_augmenter/public/test_constants.ts	`100.00% <ø> (ø)`
...ins/vis_type_vega/public/data_model/vega_parser.ts	`72.18% <0.00%> (-5.87%)`	⬇️
...plugins/vis_type_vega/public/vega_visualization.js	`76.66% <ø> (ø)`
...plugins/visualizations/public/embeddable/events.ts	`100.00% <ø> (ø)`
...c/plugins/visualizations/public/expressions/vis.ts	`0.00% <0.00%> (ø)`
src/plugins/vis_augmenter/public/vega/helpers.ts	`93.33% <28.57%> (-4.63%)`	⬇️
...s/vis_type_vega/public/vega_view/vega_base_view.js	`50.29% <50.00%> (-0.01%)`	⬇️
...actions/public/triggers/external_action_trigger.ts	`100.00% <100.00%> (ø)`

... and 6 files with indirect coverage changes

src/plugins/vis_augmenter/public/vega/constants.ts

src/plugins/vis_augmenter/public/vega/helpers.ts

ohltyler · 2023-04-10T18:24:22Z

LGTM regarding the clicking event handling framework! As part of the issue #3317 , can you also add a handler for custom tooltips? The content of the tooltips can be TODO for now and I can wrap up in a separate PR.

And as a follow-up, can we add a flag somewhere to disable a particular event (e.g., clicking) from happening via the vis config somewhere? As part of the view events flyout, we will want to disable clicking on the charts to prevent endless flyout opening.

joshuarrrr

This mostly looks good on first read-through - just some minor nits about naming, and I agree with @ohltyler's suggestions.

How can this be tested or verified, though? There don't really seem to be any test cases, but maybe this is something targeted for functional tests. In any case some screencasts would help for other reviewers.

src/plugins/vis_type_vega/public/data_model/types.ts

src/plugins/vis_augmenter/public/vega/helpers.ts

src/plugins/vis_type_vega/public/vega_view/vega_base_view.js

Signed-off-by: Amardeepsingh Siglani <amardeep7194@gmail.com>

amsiglan · 2023-04-26T05:15:40Z

LGTM regarding the clicking event handling framework! As part of the issue #3317 , can you also add a handler for custom tooltips? The content of the tooltips can be TODO for now and I can wrap up in a separate PR.

And as a follow-up, can we add a flag somewhere to disable a particular event (e.g., clicking) from happening via the vis config somewhere? As part of the view events flyout, we will want to disable clicking on the charts to prevent endless flyout opening.

For the disable piece, I think it would be more convenient to check whether the flyout is already open and then no-op inside the flyout code than trying to disable it via config. Thoughts?

Signed-off-by: Amardeepsingh Siglani <amardeep7194@gmail.com>

ohltyler · 2023-04-26T20:59:13Z

LGTM regarding the clicking event handling framework! As part of the issue #3317 , can you also add a handler for custom tooltips? The content of the tooltips can be TODO for now and I can wrap up in a separate PR.
And as a follow-up, can we add a flag somewhere to disable a particular event (e.g., clicking) from happening via the vis config somewhere? As part of the view events flyout, we will want to disable clicking on the charts to prevent endless flyout opening.

For the disable piece, I think it would be more convenient to check whether the flyout is already open and then no-op inside the flyout code than trying to disable it via config. Thoughts?

My point with this is, we would need somewhere to pass logic to the chart here to disable clicks / no-op. Do you have a suggestion on how to do that based on your changes?

Given some of the framework needed as part of the View Events flyout, we will be able to pass sufficient context to the chart here where we can disable the action.

Signed-off-by: Amardeepsingh Siglani <amardeep7194@gmail.com>

ohltyler

LGTM! Thanks again for helping out here!

ashwin-pc · 2023-04-26T22:22:16Z

I have a high level concern with this approach. Expressions today pass interaction data through ExpressionRendererEvents. This bypasses that abstraction all together and calls for separate callbacks based on the spec. I think we should definitely provide a provision in the spec to define the events we want to support but rely on the Renderer Event bus to trigger and capture events using a common event bus.

This is how all VisBuilder events for example are handled today irrespective of the underlying renderer:https://github.com/opensearch-project/OpenSearch-Dashboards/blob/main/src/plugins/vis_builder/public/application/components/workspace.tsx#L119. It then takes this event data and retransmits it using UI actions that and UI action handler can then listen to and perform an action.

The flow that i would expect here is something like this:

Use the existing vegalite selection spec to augment the desired selections we want to capture, or add functionality to extend the spec to help you define the props needed for the event. e.g. event name and captured data https://vega.github.io/vega-lite-v2/docs/selection.html
Add feature to vegalite expression function to pass these interactions to the Expression renderer event bus so that existing event handlers for the expression loader can pass these events to the UI Actions plugin and emmit them across the app
Register an UI actions handler for the alert actions in the Alerting plugin that will trigger a flyout on the Dashboard page with additional context for the event in question.

ashwin-pc · 2023-04-26T22:35:14Z

Also based on what i learnt from @joshuarrrr it looks like the vega expression function is already setup to handle the the click and brush events, and my gut feeling is that its using a pipeline similar to the one ive described above. Doing a deep dive on how that works could probably answer how we want to implement the same here.

…dd-annotation-click-handler

Signed-off-by: Amardeepsingh Siglani <amardeep7194@gmail.com>

src/plugins/vis_augmenter/public/vega/helpers.test.ts

src/plugins/vis_type_vega/public/vega_view/vega_base_view.js

ohltyler · 2023-05-11T17:38:55Z

src/plugins/vis_augmenter/public/actions/external_action_action.ts

+ execute: async (context: ExternalActionActionContext) => {
+ if (isPointInTimeAnnotation(context.data.item)) {
+ if (context.data.event === 'click') {
+ // TODO: show events flyout
+ } else if (context.data.event === 'mouseover') {
+ // TODO: show custom tooltip
+ }
+ }
+ },


Should we have these actions split into 2? One for triggering the flyout opening, and another for triggering the custom tooltip? Curious of thoughts from @ashwin-pc on this as well.

Depends on whether we want to keep events from the visualization generic and have the handler distinguish based off of the data. This way we will keep the events minimal and avoid adding specific info into the general framework.

Yeah - I don't have a strong preference either way. Agreed we don't want to overload too specific of events here. Maybe one per VisLayer implementation makes sense - in this particular case, handling click/mouseover/mouseout actions for the PointInTimeEventsVisLayer.

On second thought, perhaps we do want to have these split up. We will need a mechanism for disabling the click event behavior, but still enabling the custom tooltip behavior in the view events flyout. Do you see a way we could pass that logic here? Or if we split into 2 events maybe (1 for custom click event, 1 for custom hover event), be able to disable one and enable the other?

The actions dont need to be defined here. The plugin listen for the trigger directly. If we have a standard contract for triggers (Which this Pr is adding), the plugin can simply listen for that trigger and do as they please.

@amsiglan thanks. This makes sense to me in the context of the click event; I can defined a UIAction that listens for this trigger to open the view events flyout. But there's 2 things I'd like to clarify:

For the scenario of disabling the clicking, but still enabling the custom tooltips, how do you propose I do this? One solution I could see is altering addVisEventSignalsToSpecConfig() to only have on for the mouse events, and just omit the click event.

This is maybe a bit of a miss on my part when reviewing the latest revisions made, but for the tooltips, how should we get sufficient context passed such that a UIAction can render the tooltip correctly? The original proposal was making changes to the implemented TooltipHandler class, which is using the out-of-the-box tooltip() function provided by vega to have a customized tooltip to render it OUI-style. It requires a lot of extra context, such as the container the vega view is in, the view itself, and other positioning parameters. I actually still think we may want to treat the tooltip action as just a param within the vis itself, that determines what tooltip html we render. The example I originally had is seen here

For 1. We don't need to disable the clicks, we could just check if the flyout is already open then just no-op in the event listener.
For 2. we will need to do a quick POC and check if the provided event has all the information we need. I see it has the view and the position related information

We don't need to disable the clicks, we could just check if the flyout is already open then just no-op in the event listener.

How would you propose this? We don't persist global flyout state so how would we be able to make such a check within the UIAction?

@ohltyler Why do we need to disable click? what is the interaction pattern that we want to achieve with that?

So user can't infinitely re-open the flyout when clicking - details in the issue #3317

ashwin-pc

Based on the code change and some personal digging into this I think you might be able to make this wayy less specific to your implementation and a safer change. The way Kibana had done this seems to be using signals and vega expression functions. This is what i was able to figure out:

They register each of the vega expression functions in the base view file (But not in the class): https://github.com/ashwin-pc/OpenSearch-Dashboards/blob/main/src/plugins/vis_type_vega/public/vega_view/vega_base_view.js#L56-L63
The registered functions when called by the signals api in the spec forward the event to the instance specific handler defined in the base class Ref. This is usually done using the on -> update props for signals. I presume this is done so that the specific functions like brush have access to the correct this value.
The specific handler function then calls vis expression function that has access to the event bus and the eventSubject object that then passes the data to the event bus using the appropriate event name (e.g.applyFilter)

For your implementation you can keep it quite simple, something like this:

Add a new function here like opensearchDashboardsTrigger: 'triggerEventHandler' and define the event handler in the base class like

async triggerEventHandler(triggerName, data) {
    this._triggerEvent({ name: triggerName, data });
  }

and also add a prop for the Vega Base view to pass _triggerEvent similar to _applyFilter

Then in vega_siualization.js pass the triggerEvent from this._vis.API.events. the same way applyFilter does.
Then in the vis.ts file in expressions, add the triggerEvent function that can pass the name and data to the expression event bus (eventSubject)
From then on it should just be a matter of listening for the trigger in the AD plugin and showing the flyout based on it :)

Now to use this new function you will have to use signals. You can refer to the vega functional test to see how these signals are written and used: https://github.com/ashwin-pc/OpenSearch-Dashboards/blob/main/test/functional/apps/visualize/_vega_chart.ts#L35-L48

ashwin-pc · 2023-05-16T00:16:36Z

src/plugins/vis_augmenter/public/actions/external_action_action.ts

+ execute: async (context: ExternalActionActionContext) => {
+ if (isPointInTimeAnnotation(context.data.item)) {
+ if (context.data.event === 'click') {
+ // TODO: show events flyout
+ } else if (context.data.event === 'mouseover') {
+ // TODO: show custom tooltip
+ }
+ }
+ },


The actions dont need to be defined here. The plugin listen for the trigger directly. If we have a standard contract for triggers (Which this Pr is adding), the plugin can simply listen for that trigger and do as they please.

src/plugins/vis_type_vega/public/vega_view/vega_base_view.js

Signed-off-by: Amardeepsingh Siglani <amardeep7194@gmail.com>

src/plugins/ui_actions/public/types.ts

Signed-off-by: Amardeepsingh Siglani <amardeep7194@gmail.com>

ohltyler

LGTM! We can keep the tracking issue open until tooltip handling is merged :)

ashwin-pc

Nice this is definitely almost ready, just had a few questions

ashwin-pc · 2023-05-18T20:37:43Z

src/plugins/vis_augmenter/public/vega/helpers.ts

+ const config = get(spec, 'config', { kibana: {} });
+ const signals = {
+ ...(config.kibana.signals || {}),
+ [`${VisAnnotationType.POINT_IN_TIME_ANNOTATION}`]: [


Arent signals an array?

e.g.

"signals": [ { "name": "indexDate", "description": "A date value that updates in response to mousemove.", "update": "datetime(2005, 0, 1)", "on": [{"events": "mousemove", "update": "invert('xscale', x())"}] } ],

This is just a signals config that isn't directly tied to the spec. It's embedded within the existing kibana field in the vega lite spec that's processed to handle OSD-specific items in the vega parser.

From my understanding signals here is just a mapping of mark ID to a signals array (that will be added to the compiled vega spec later on)

Yes, that is correct @ohltyler . In the vega parser we use the key "markId" to find the mark and then add then update the signal with that mark name.

ashwin-pc · 2023-05-18T20:42:42Z

src/plugins/vis_type_vega/public/data_model/vega_parser.ts

@@ -323,6 +324,52 @@ The URL is an identifier only. OpenSearch Dashboards and your browser will never
 delete this.spec.autosize;
 }
 }
+
+ if (this._config?.signals) {
+ Object.entries(this._config?.signals).forEach(([markId, signals]: [string, any]) => {


Just to clarify will this break any existing spec that uses signals?

No. Since we control this signals value here, as it's set by us in addVisEventSignalsToSpecConfig(). signals here isn't the actual signals field per the vega spec, but just the config field name embedded within kibana config obj. See comment above.

ashwin-pc

Nice this is definitely almost ready, just had a few questions

ashwin-pc

Thanks for all the context and changes @amsiglan, this looks good to me!

joshuarrrr

We could use more testing of edge case handling.

joshuarrrr · 2023-05-22T21:47:01Z

src/plugins/visualizations/public/expressions/vis.ts

@@ -99,6 +100,10 @@ export class ExprVis extends EventEmitter {
 if (!this.eventsSubject) return;
 this.eventsSubject.next({ name: 'applyFilter', data });
 },
+ externalAction: (data: any) => {


Is this planned for functional testing, or can it be unit tested?

joshuarrrr · 2023-05-22T21:49:17Z

src/plugins/vis_type_vega/public/data_model/vega_parser.ts

+ * This method recursively looks for a mark that includes the given style.
+ * Returns undefined if it doesn't find it.
+ */
+ getMarkWithStyle(marks: any[], style: string): any {


Seems like some unit tests here would be nice to validate all the conditional branching.

amsiglan added 2 commits April 4, 2023 00:12

added interaction handling

edab515

Signed-off-by: Amardeepsingh Siglani <amardeep7194@gmail.com>

fixed lint errors

5fca51a

Signed-off-by: Amardeepsingh Siglani <amardeep7194@gmail.com>

amsiglan requested a review from a team as a code owner April 4, 2023 07:21

joshuarrrr added the feature-anywhere label Apr 5, 2023

amsiglan added 2 commits April 4, 2023 17:42

fixed unit tests

0fe8e82

Signed-off-by: Amardeepsingh Siglani <amardeep7194@gmail.com>

fixed unit test

325c9fc

Signed-off-by: Amardeepsingh Siglani <amardeep7194@gmail.com>

lezzago approved these changes Apr 5, 2023

View reviewed changes

ohltyler reviewed Apr 10, 2023

View reviewed changes

src/plugins/vis_augmenter/public/vega/constants.ts Outdated Show resolved Hide resolved

ohltyler reviewed Apr 10, 2023

View reviewed changes

src/plugins/vis_augmenter/public/vega/helpers.ts Outdated Show resolved Hide resolved

joshuarrrr self-assigned this Apr 13, 2023

ohltyler mentioned this pull request Apr 19, 2023

[Feature Anywhere] Enable plugins to augment visualizations with additional data and context #2880

Closed

31 tasks

joshuarrrr reviewed Apr 21, 2023

View reviewed changes

src/plugins/vis_type_vega/public/data_model/types.ts Outdated Show resolved Hide resolved

src/plugins/vis_augmenter/public/vega/helpers.ts Outdated Show resolved Hide resolved

src/plugins/vis_type_vega/public/vega_view/vega_base_view.js Outdated Show resolved Hide resolved

This was referenced Apr 21, 2023

[Feature Anywhere] Support clicking & custom tooltip functionality from vega charts #3317

Closed

Add custom tooltips and clicking functionality to vega charts #3369

Closed

addressed comments

09d6cbb

Signed-off-by: Amardeepsingh Siglani <amardeep7194@gmail.com>

amsiglan requested review from ananzh, kavilla, seanneumann, AMoo-Miki, ashwin-pc, abbyhu2000, zengyan-amazon, kristenTian, zhongnansu and manasvinibs as code owners April 26, 2023 05:13

updated test constants

e4921c6

Signed-off-by: Amardeepsingh Siglani <amardeep7194@gmail.com>

amsiglan added 2 commits April 26, 2023 14:52

addressed PR comments

5912441

Signed-off-by: Amardeepsingh Siglani <amardeep7194@gmail.com>

addressed PR comments

76d8744

Signed-off-by: Amardeepsingh Siglani <amardeep7194@gmail.com>

ohltyler approved these changes Apr 26, 2023

View reviewed changes

lezzago added the v2.8.0 label May 4, 2023

ashwin-pc mentioned this pull request May 4, 2023

2.8.0 Platform Upgrades for Plugins (Feature Anywhere, Point in time, Node 18,+) #3981

Closed

9 tasks

amsiglan added 2 commits May 11, 2023 01:07

Merge remote-tracking branch 'origin/feature/feature-anywhere' into a…

e469fe9

…dd-annotation-click-handler

refactored to use event bus for notifying click on annotation

ed817b2

Signed-off-by: Amardeepsingh Siglani <amardeep7194@gmail.com>

amsiglan force-pushed the add-annotation-click-handler branch from 1093a3f to ed817b2 Compare May 11, 2023 08:08

added framework for invoking flyout, tooltip

a53f8ab

Signed-off-by: Amardeepsingh Siglani <amardeep7194@gmail.com>

amsiglan commented May 11, 2023

View reviewed changes

src/plugins/vis_augmenter/public/vega/helpers.test.ts Outdated Show resolved Hide resolved

ohltyler reviewed May 11, 2023

View reviewed changes

src/plugins/vis_type_vega/public/vega_view/vega_base_view.js Outdated Show resolved Hide resolved

ohltyler reviewed May 11, 2023

View reviewed changes

ashwin-pc reviewed May 16, 2023

View reviewed changes

amsiglan added 2 commits May 16, 2023 19:52

using signals to listen for events on point annotation

618ef27

Signed-off-by: Amardeepsingh Siglani <amardeep7194@gmail.com>

updated test constant

4669fca

Signed-off-by: Amardeepsingh Siglani <amardeep7194@gmail.com>

ohltyler reviewed May 17, 2023

View reviewed changes

src/plugins/ui_actions/public/types.ts Outdated Show resolved Hide resolved

amsiglan added 2 commits May 17, 2023 15:02

simplified logic to only include click event

67cc5f5

Signed-off-by: Amardeepsingh Siglani <amardeep7194@gmail.com>

removed unused code

6d9b9b4

Signed-off-by: Amardeepsingh Siglani <amardeep7194@gmail.com>

ohltyler approved these changes May 17, 2023

View reviewed changes

ashwin-pc reviewed May 18, 2023

View reviewed changes

ohltyler mentioned this pull request May 19, 2023

[Feature Anywhere] Add view events flyout #3415

Merged

18 tasks

ashwin-pc approved these changes May 22, 2023

View reviewed changes

joshuarrrr approved these changes May 22, 2023

View reviewed changes

joshuarrrr merged commit e7e4b2a into opensearch-project:feature/feature-anywhere May 22, 2023

kavilla added v2.9.0 and removed v2.8.0 labels Jun 1, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Feature anywhere] Add annotation click handler #3777

[Feature anywhere] Add annotation click handler #3777

amsiglan commented Apr 4, 2023

joshuarrrr commented Apr 5, 2023

codecov-commenter commented Apr 5, 2023 •

edited by codecov bot

Loading

ohltyler commented Apr 10, 2023 •

edited

Loading

joshuarrrr left a comment

amsiglan commented Apr 26, 2023

ohltyler commented Apr 26, 2023

ohltyler left a comment

ashwin-pc commented Apr 26, 2023 •

edited

Loading

ashwin-pc commented Apr 26, 2023 •

edited

Loading

ohltyler May 11, 2023

amsiglan May 11, 2023

ohltyler May 11, 2023

ohltyler May 11, 2023

ashwin-pc May 16, 2023

ohltyler May 17, 2023 •

edited

Loading

amsiglan May 17, 2023

ohltyler May 17, 2023 •

edited

Loading

ashwin-pc May 17, 2023

ohltyler May 17, 2023

ashwin-pc left a comment

ashwin-pc May 16, 2023

ohltyler left a comment

ashwin-pc left a comment

ashwin-pc May 18, 2023

ohltyler May 18, 2023 •

edited

Loading

amsiglan May 18, 2023

ashwin-pc May 18, 2023

ohltyler May 18, 2023

ashwin-pc left a comment

ashwin-pc left a comment

joshuarrrr left a comment

joshuarrrr May 22, 2023

joshuarrrr May 22, 2023

[Feature anywhere] Add annotation click handler #3777

[Feature anywhere] Add annotation click handler #3777

Conversation

amsiglan commented Apr 4, 2023

Description

Issues Resolved

Check List

joshuarrrr commented Apr 5, 2023

codecov-commenter commented Apr 5, 2023 • edited by codecov bot Loading

Codecov Report

ohltyler commented Apr 10, 2023 • edited Loading

joshuarrrr left a comment

Choose a reason for hiding this comment

amsiglan commented Apr 26, 2023

ohltyler commented Apr 26, 2023

ohltyler left a comment

Choose a reason for hiding this comment

ashwin-pc commented Apr 26, 2023 • edited Loading

ashwin-pc commented Apr 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ohltyler May 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ohltyler May 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ashwin-pc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ohltyler left a comment

Choose a reason for hiding this comment

ashwin-pc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ohltyler May 18, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ashwin-pc left a comment

Choose a reason for hiding this comment

ashwin-pc left a comment

Choose a reason for hiding this comment

joshuarrrr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Apr 5, 2023 •

edited by codecov bot

Loading

ohltyler commented Apr 10, 2023 •

edited

Loading

ashwin-pc commented Apr 26, 2023 •

edited

Loading

ashwin-pc commented Apr 26, 2023 •

edited

Loading

ohltyler May 17, 2023 •

edited

Loading

ohltyler May 17, 2023 •

edited

Loading

ohltyler May 18, 2023 •

edited

Loading