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 logic to collect VisLayers in line chart render flow #3131

Conversation

ohltyler
Copy link
Member

@ohltyler ohltyler commented Dec 23, 2022

Description

This PR adds the logic for collecting & parsing VisLayers to eventually be read by the downstream render expressions functions. More details can be found in the related issue

Most of the new logic is happening in the fetchVisLayers() function of visualize_embeddable (it is calling new helper functions added in utils). This is ran whenever there are changes that affect the embeddable such that it needs to be re-rendered. In our use case, we will want to dynamically fetch updated VisLayers whenever there are such changes, since the layers themselves are dynamically generated based on the context for the existing visualization (time range, filter, query). After any VisLayers are collected, they are passed to the final buildPipeline fn which constructs the end-to-end string pipeline to execute and render the entire visualization.

The final processing of the VisLayers will be handled in 2 more PRs:

  1. Setting up the toExpressionAst() fn for line chart visualizations as part of Create switch to render line charts using vega-lite #3106
  2. Add logic in the new expressions function to parse any VisLayers and dynamically update the vega spec as part of Dynamically update vega spec to show VisLayers #3145

Other things to note:

  1. There are some housekeeping changes where files from vis_augmenter/common are moved to vis_augmenter/public. This affects the files themselves + where they were being imported. There is no changes to the content of the files, so they don't need much review/attention
  2. The isEligibleForVisLayers() helper fn is a placeholder for now - deeper logic will be added for that in the related issue [Feature Anywhere] Finalize eligibility based on vis configuration #3268. I've linked that in the code, and will also add a note in that issue to make sure this is tracked properly

Issues Resolved

Closes #3122

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
  • [s] Commits are signed per the DCO using --signoff

@ohltyler ohltyler requested a review from a team as a code owner December 23, 2022 01:26
@ohltyler ohltyler changed the title Add layer to embeddable Add logic to collect VisLayers in line chart render flow Dec 23, 2022
@ohltyler ohltyler added the Skip-Changelog PRs that are too trivial to warrant a changelog or release notes entry label Dec 23, 2022
@ohltyler ohltyler changed the base branch from main to feature/feature-anywhere December 23, 2022 01:27
@ohltyler
Copy link
Member Author

Some of the new imports won't resolve until earlier PRs have been merged and I can rebase here. Until then you can ignore those TODO comments

@codecov-commenter
Copy link

codecov-commenter commented Dec 23, 2022

Codecov Report

Merging #3131 (f4a6386) into feature/feature-anywhere (d63ef1f) will decrease coverage by 0.07%.
The diff coverage is 61.76%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@                     Coverage Diff                      @@
##           feature/feature-anywhere    #3131      +/-   ##
============================================================
- Coverage                     66.59%   66.53%   -0.07%     
============================================================
  Files                          3212     3216       +4     
  Lines                         61468    61506      +38     
  Branches                       9474     9476       +2     
============================================================
- Hits                          40937    40924      -13     
- Misses                        18273    18319      +46     
- Partials                       2258     2263       +5     
Flag Coverage Δ
Linux 66.53% <61.76%> (-0.01%) ⬇️
Windows ?

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

Impacted Files Coverage Δ
.../plugins/vis_augmenter/public/expressions/types.ts 50.00% <ø> (ø)
src/plugins/vis_augmenter/public/index.ts 0.00% <ø> (ø)
...nter/public/saved_augment_vis/saved_augment_vis.ts 100.00% <ø> (ø)
...ic/embeddable/create_vis_embeddable_from_object.ts 5.26% <0.00%> (-0.30%) ⬇️
...izations/public/embeddable/visualize_embeddable.ts 0.63% <0.00%> (-0.05%) ⬇️
...ins/visualizations/public/legacy/build_pipeline.ts 46.28% <ø> (ø)
src/plugins/vis_augmenter/public/utils/utils.ts 93.33% <93.33%> (ø)
...ter/public/saved_augment_vis/utils/test_helpers.ts 100.00% <100.00%> (ø)
src/plugins/vis_augmenter/public/types.ts 100.00% <100.00%> (ø)
src/plugins/visualizations/public/services.ts 100.00% <100.00%> (ø)
... and 11 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@ohltyler
Copy link
Member Author

ohltyler commented Feb 9, 2023

PR is rebased and ready. Please read PR description before starting the review

@@ -138,6 +147,7 @@ export class VisualizeEmbeddable
VisualizeByReferenceInput
>,
savedVisualizationsLoader?: SavedVisualizationsLoader,
savedAugmentVisLoader?: SavedAugmentVisLoader,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: We can declare this private here which will combine lines 138 and 173 below
private savedAugmentVisLoader?: SavedAugmentVisLoader

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure what you mean here. It is declared private on 138, and line 150 is just specifying an arg in the constructor. I was following the patterns for the rest of the visualize_embeddable private fields, and am consistent with the existing savedVisualizationsLoader

objs.forEach((obj: ISavedAugmentVis) => {
visLayerExpressionFns.push(
buildExpressionFunction<VisLayerFunctionDefinition>(
obj.visLayerExpressionFn.name,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Doubt: We call this as expressionFn but I am not sure if it has an actual function implementation associated with it because from the type I see it has a name and args associated with it. Can you point me to a place that can help me understand this better? Thanks!

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure - it's defined in src/plugins/vis_augmenter/public/types.ts in #3109. Essentially it's an interface that defines how to run a plugin-defined expression function. type represents the return type of the expression function (same as existing expression functions), name is the name of the registered function, and args is the loosely-typed field for plugins to add their own args. An example would be something like an add_anomalies function defined in the AD plugin, which would collect anomalies and return the return type.

These functions are then read here and parsed out to create a single expressions string pipeline, where when executed, will collect all VisLayers.

See details and diagrams in the meta issue #2880

Copy link
Collaborator

@amsiglan amsiglan left a comment

Choose a reason for hiding this comment

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

Looks good to me overall.

@joshuarrrr joshuarrrr removed the v2.6.0 label Feb 17, 2023
Signed-off-by: Tyler Ohlsen <ohltyler@amazon.com>
@abbyhu2000
Copy link
Member

@ohltyler I re-ran the tests couple times, seem like it is failing some of the vis builder functional tests.

1)    visBuilder app
     │       Basic tests for visBuilder app 
     │         should show visualization when a field is added:
     │
     │      StaleElementReferenceError: stale element reference: element is not attached to the page document
     │   (Session info: headless chrome=110.0.5481.77)
  
     │2)    visBuilder app
     │       Basic tests for visBuilder app 
     │         should clear visualization when field is deleted:
     │
     │      Error: retry.try timeout: TimeoutError: Waiting for element to be located By(css selector, [data-test-subj="dropBoxField-metric-0"] [data-test-subj="dropBoxRemoveBtn"])

@ohltyler
Copy link
Member Author

ohltyler commented Mar 2, 2023

@ohltyler I re-ran the tests couple times, seem like it is failing some of the vis builder functional tests.

1)    visBuilder app
     │       Basic tests for visBuilder app 
     │         should show visualization when a field is added:
     │
     │      StaleElementReferenceError: stale element reference: element is not attached to the page document
     │   (Session info: headless chrome=110.0.5481.77)
  
     │2)    visBuilder app
     │       Basic tests for visBuilder app 
     │         should clear visualization when field is deleted:
     │
     │      Error: retry.try timeout: TimeoutError: Waiting for element to be located By(css selector, [data-test-subj="dropBoxField-metric-0"] [data-test-subj="dropBoxRemoveBtn"])

this is unrelated to our change and could be due to the ongoing visbuilder changes and what is currently merged in our feature branch. I think this can be ignored and we can ensure all tests are passing consistently when finally merging into main/2.x.

cc @joshuarrrr who is tracking any errors that pop up in these runs

Copy link
Member

@abbyhu2000 abbyhu2000 left a comment

Choose a reason for hiding this comment

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

LGTM! Just commented a couple nits.

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

ohltyler commented Mar 7, 2023

@abbyhu2000 I've refactored a few of the types & interfaces into separate types.ts files so they are more organized. The only change was making the VisLayerFunctionDefinition more typesafe by using the existing ExprVisLayers as the input/output.

Copy link
Member

@abbyhu2000 abbyhu2000 left a comment

Choose a reason for hiding this comment

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

@ohltyler Thank you for addressing the comments!

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.

Some nits and code style suggestions, but no blockers. I think using find instead of findAll would be the most significant improvement.

Also, thanks @ohltyler for the useful PR description!

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.

one linting failure - you should be able to accept my change, and then I'll re-approve.

…ble.ts


Signed-off-by: Josh Romero <rmerqg@amazon.com>
@joshuarrrr joshuarrrr merged commit 236f49f into opensearch-project:feature/feature-anywhere Mar 9, 2023
@ohltyler ohltyler deleted the add-layer-to-embeddable branch March 9, 2023 00:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-anywhere Skip-Changelog PRs that are too trivial to warrant a changelog or release notes entry v2.9.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature Anywhere] Collect VisLayers when rendering visualization line charts
9 participants