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 inspector for VEGA #70941

Merged
merged 42 commits into from
Jul 21, 2020
Merged

Add inspector for VEGA #70941

merged 42 commits into from
Jul 21, 2020

Conversation

alexwizp
Copy link
Contributor

@alexwizp alexwizp commented Jul 7, 2020

Closes: #31189, #71413

Summary

Add Inspector Panel for Vega Visualization

Request tab:

image

Vega Debug tab:

image
image
image

Checklist

Delete any items that are not applicable to this PR.

For maintainers

@alexwizp alexwizp added the WIP Work in progress label Jul 7, 2020
@timroes
Copy link
Contributor

timroes commented Jul 7, 2020

Kibana Pipeline / kibana-oss-agent / Chrome UI Functional Tests.test/functional/apps/visualize/_vega_chart·js.visualize app vega chart in visualize app vega chart initial render should not have inspector enabled

Sounds like a good test to fail 🎉

@alexwizp alexwizp force-pushed the 31189 branch 2 times, most recently from 57bd6bd to 392a39e Compare July 8, 2020 08:33
@alexwizp alexwizp requested a review from ppisljar July 8, 2020 14:06
@alexwizp alexwizp requested a review from nyurik July 8, 2020 14:35
@nyurik
Copy link
Contributor

nyurik commented Jul 8, 2020

This looks awesome, so happy it is happening!!! There are a few items we may want to consider:

  • When users ask questions or experiment with the official Vega editor, they follow my guide to run JSON.stringify(VEGA_DEBUG.vega_spec) or JSON.stringify(VEGA_DEBUG.vegalite_spec) to convert their code into a pure JSON with the raw data. We probably should offer this, possibly as a new tab?
  • there could be more than one ES request - how do you show that in the inspector?
  • Vega supports dynamic data sources (we haven't implemented that yet) - so it is possible for some user actions to cause an additional ES request(s) to be made. Please consider how to handle that (we don't have to support it from the start)
  • if it is possible to hold the inspector window open while experimenting with a Vega graph, the signals should auto-refresh (e.g. on timer?), ideally with highlighting of the changed value (like in an IDE to show which signal actually changed)

Thanks!

@nyurik
Copy link
Contributor

nyurik commented Jul 8, 2020

P.S. Visuals - for the signals and data set tabs, I don't think we actually need the labes "Data:", "View:", and "Signals:" -- seems self evident, and takes up space. Should probably do it the same way as for request/response -- no extra labeling there.

@nyurik
Copy link
Contributor

nyurik commented Jul 8, 2020

P.P.S. Sorry for the nit -- would it make sense to make general inspector changes in a separate PR? Seems a bit confusing to do parameter refactoring and other general inspector work together with the vega inspector.

@alexwizp
Copy link
Contributor Author

alexwizp commented Jul 9, 2020

  • there could be more than one ES request - how do you show that in the inspector?

Request panel logs all requests separately. It should looks something like:

image

@alexwizp
Copy link
Contributor Author

@elasticmachine merge upstream

@alexwizp
Copy link
Contributor Author

@elasticmachine merge upstream

@timroes
Copy link
Contributor

timroes commented Jul 13, 2020

Currently blocked by #71413

Copy link
Contributor

@wylieconlon wylieconlon left a comment

Choose a reason for hiding this comment

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

The behavior is working as expected. The size of the Signals inspector makes it hard to use for debugging large JSON objects, and I've suggested some improvements to this which would make it a little easier- but these are optional.

@alexwizp
Copy link
Contributor Author

@ppisljar could you please review this PR again?

@alexwizp
Copy link
Contributor Author

@elasticmachine merge upstream

@alexwizp
Copy link
Contributor Author

@elasticmachine merge upstream

cchaos added 2 commits July 20, 2020 12:49
- Fixing up layout trying to remove any `.eui` classes and uses flex instead of percentage
- Fixing text to use `Sentence case` not `Title Case`
Copy link
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

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

👍 LGTM from Design. Tested in Chrome and Firefox. I've added some notes about todo's for EUI to supply adding classes to a few internal components, but at least they aren't targeting EUI classes anymore.

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Build metrics

@kbn/optimizer bundle module count

id value diff baseline
inspector 49 +10 39
visTypeVega 197 +9 188
total - +19 -

async chunks size

id value diff baseline
visTypeVega 1.4MB +344.0B 1.4MB

page load bundle size

id value diff baseline
data 1.4MB +52.0B 1.4MB
expressions 343.4KB +94.0B 343.3KB
inputControlVis 295.2KB +21.0B 295.2KB
inspector 278.8KB +3.7KB 275.1KB
visTypeMarkdown 552.8KB +21.0B 552.7KB
visTypeTimelion 712.5KB +21.0B 712.5KB
visTypeTimeseries 280.2KB +21.0B 280.2KB
visTypeVega 660.6KB +33.2KB 627.4KB
visualizations 407.3KB +352.0B 407.0KB
total - +37.4KB -

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

Copy link
Member

@ppisljar ppisljar left a comment

Choose a reason for hiding this comment

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

app arch code LGTM

@alexwizp alexwizp merged commit e1ffccc into elastic:master Jul 21, 2020
gmmorris added a commit to gmmorris/kibana that referenced this pull request Jul 21, 2020
* master: (28 commits)
  allow some env settings for ingest manager (elastic#72544)
  Add inspector for VEGA (elastic#70941)
  chore(NA): fix grunt task for test:coverage (elastic#72539)
  Archive e2e test results in ES (elastic#72575)
  preserve 401 errors from new es client (elastic#71248)
  [SIEM][Detections] Updates text for severity and risk_score overrides  (elastic#72244)
  fixing error occurences tooltip (elastic#72425)
  use KibanaClient interface instead of Client for new client interface (elastic#72388)
  [APM] Handle ML errors (elastic#72316)
  [Discover] Improve histogram tests (elastic#72235)
  [ftr/webdriver] retry on all errors, use Rx so that timers are canceled (elastic#72540)
  [pre-req] Move .storybook to storybook; standardize files (elastic#72384)
  [Security_Solution][Resolver][Bug]: Restore breadcrumb background (elastic#72538)
  [ML] Fix annotation detector linking & delayed_data(0) (elastic#72468)
  [Security Solution][Exceptions] - Make esTypes and subType available to index patterns (elastic#72336)
  [SIEM] Uses faster wait from testing-library and removes duplicate older wait idiom (elastic#72509)
  Fix long combo box items breaking out of flex item width (elastic#72512)
  [pipeline/commitStatus] update commit status in baseline-capture job (elastic#72366)
  [Security Solution][Resolver] Update the resolver element ref on scroll events if the position of the element has changed within the page (elastic#72461)
  [Maps] auto-fit to data bounds (elastic#72129)
  ...
gmmorris added a commit to gmmorris/kibana that referenced this pull request Jul 21, 2020
…feature-privileges

* alerting/consumer-based-rbac: (30 commits)
  removed uneeded tests
  expclude security wrapper in SO client passed to ActionsClient
  allow some env settings for ingest manager (elastic#72544)
  Add inspector for VEGA (elastic#70941)
  chore(NA): fix grunt task for test:coverage (elastic#72539)
  Archive e2e test results in ES (elastic#72575)
  preserve 401 errors from new es client (elastic#71248)
  [SIEM][Detections] Updates text for severity and risk_score overrides  (elastic#72244)
  fixing error occurences tooltip (elastic#72425)
  use KibanaClient interface instead of Client for new client interface (elastic#72388)
  [APM] Handle ML errors (elastic#72316)
  [Discover] Improve histogram tests (elastic#72235)
  [ftr/webdriver] retry on all errors, use Rx so that timers are canceled (elastic#72540)
  [pre-req] Move .storybook to storybook; standardize files (elastic#72384)
  [Security_Solution][Resolver][Bug]: Restore breadcrumb background (elastic#72538)
  [ML] Fix annotation detector linking & delayed_data(0) (elastic#72468)
  [Security Solution][Exceptions] - Make esTypes and subType available to index patterns (elastic#72336)
  [SIEM] Uses faster wait from testing-library and removes duplicate older wait idiom (elastic#72509)
  Fix long combo box items breaking out of flex item width (elastic#72512)
  [pipeline/commitStatus] update commit status in baseline-capture job (elastic#72366)
  ...
alexwizp added a commit to alexwizp/kibana that referenced this pull request Jul 21, 2020
* [WIP] Add inspector for VEGA

Closes: elastic#31189

* view -> dataset

* cleanup

* add spec viewer

* cleanup code

* use rx to retrieve data from adapters

* Make custom inspector adapters registerable from the visType

* fix flex-box size

* cleanup

* remove visTypesWithoutInspector from visualize_embeddable

* fix PR comments

* add vega folder to sass-lint

* fix jest

* Update src/plugins/vis_type_vega/public/vega_inspector/components/data_viewer.tsx

Co-authored-by: Wylie Conlon <[email protected]>

* use addSignalListener

* cleanup

* add onColumnResize handler

* EuiCodeEditor -> CodeEditor

* fix type_check

* fix issue with pagination

* fix extra vertical scroll

* add area-label for EuiButtonIcon

* add area-label for EuiComboBox

* Design Commit

- Fixing up layout trying to remove any `.eui` classes and uses flex instead of percentage
- Fixing text to use `Sentence case` not `Title Case`

* Wrapper around signal viewer table

* fix Jest snapshot

Co-authored-by: Elastic Machine <[email protected]>
Co-authored-by: Wylie Conlon <[email protected]>
Co-authored-by: cchaos <[email protected]>
alexwizp added a commit that referenced this pull request Jul 21, 2020
* [WIP] Add inspector for VEGA

Closes: #31189

* view -> dataset

* cleanup

* add spec viewer

* cleanup code

* use rx to retrieve data from adapters

* Make custom inspector adapters registerable from the visType

* fix flex-box size

* cleanup

* remove visTypesWithoutInspector from visualize_embeddable

* fix PR comments

* add vega folder to sass-lint

* fix jest

* Update src/plugins/vis_type_vega/public/vega_inspector/components/data_viewer.tsx

Co-authored-by: Wylie Conlon <[email protected]>

* use addSignalListener

* cleanup

* add onColumnResize handler

* EuiCodeEditor -> CodeEditor

* fix type_check

* fix issue with pagination

* fix extra vertical scroll

* add area-label for EuiButtonIcon

* add area-label for EuiComboBox

* Design Commit

- Fixing up layout trying to remove any `.eui` classes and uses flex instead of percentage
- Fixing text to use `Sentence case` not `Title Case`

* Wrapper around signal viewer table

* fix Jest snapshot

Co-authored-by: Elastic Machine <[email protected]>
Co-authored-by: Wylie Conlon <[email protected]>
Co-authored-by: cchaos <[email protected]>

Co-authored-by: Elastic Machine <[email protected]>
Co-authored-by: Wylie Conlon <[email protected]>
Co-authored-by: cchaos <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:ExpressionLanguage Interpreter expression language (aka canvas pipeline) Feature:Inspector Inspector infrastructure and implementations Feature:Vega Vega visualizations release_note:enhancement Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.10.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add inspector to Vega vis
10 participants