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

Deprecate schema-less specs in Vega #73805

Merged
merged 10 commits into from
Aug 11, 2020
Merged

Deprecate schema-less specs in Vega #73805

merged 10 commits into from
Aug 11, 2020

Conversation

alexwizp
Copy link
Contributor

@alexwizp alexwizp commented Jul 30, 2020

Closes #30951

Release Notes:

We decided to don't support Vega visualizations without a $schema specification property.

Before that PR in case if user did not provide the $schema property, we set the default value which was hardcoded in Vega code (

for Kibana < 7.9 it was https://vega.github.io/schema/vega/v3.json
for Kibana 7.9 it was https://vega.github.io/schema/vega/v5.json

) and then rendered visualization with a warning message.

This introduced certain difficulties when updating the version of the Vega library as a result we decided to change this approach.

Now all Vega Specs should contain $schema param. In case of no $schema param user should see an error message

Please see Vega Doc get more information about this property.

image

Checklist

Delete any items that are not applicable to this PR.

For maintainers

@alexwizp alexwizp added release_note:breaking release_note:plugin_api_changes Contains a Plugin API changes section for the breaking plugin API changes section. Feature:Vega Vega visualizations labels Jul 30, 2020
@alexwizp alexwizp self-assigned this Jul 30, 2020
@alexwizp alexwizp added Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.10.0 v8.0.0 labels Jul 30, 2020
@nyurik
Copy link
Contributor

nyurik commented Jul 30, 2020

I think this is a good change. One note -- make sure the error offers "copy/pastable" schema strings, e.g. say "for Vega, use "$schema": "...", and for Vega-Lite use ... . Could also add some text saying that this link does not need to be accessible, and will only be used for describing Vega code (i kept getting this question, esp for the air-gaped customers)

@alexwizp
Copy link
Contributor Author

@nyurik np, message was updated

image

@alexwizp alexwizp marked this pull request as ready for review July 31, 2020 13:25
@alexwizp alexwizp requested a review from a team July 31, 2020 13:25
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app (Team:KibanaApp)

@nyurik
Copy link
Contributor

nyurik commented Jul 31, 2020

@nyurik np, message was updated

image

@alexwizp thanks, but I think we should iterate on it a bit more -- something like this would probably be better, as it would require less thinking (always a good thing™). We could try to get someone from the docs team involved - they are very good at this sorts of things :)

The "$schema" field is missing.
Add one of these lines to your code, depending if you use Vega or Vega-Lite:

   "$schema": "https://vega.github.io/schema/vega/v5.json"
   "$schema": "https://vega.github.io/schema/vega-lite/v4.json"

Note that the URL does not need to be accessible from your environment,
and will only be used to identify the required Vega or Vega-Lite engine.

@timroes
Copy link
Contributor

timroes commented Aug 3, 2020

I think we should not try to put specific versions in there. It just have shown in the past those get outdated. Also if you're up to writing a Vega specification, finding that $schema URL should be the least of your problems, or put otherwise: If you're not able to get the schema URL, you'll most likely not have much success with the rest of your spec.

I'd suggest we might instead just add some links to the vega documentation that talks about the spec instead, and make the message something like the following:

Your specification is missing a "$schema" field. You need to add a valid [vega](https://vega.github.io/vega/docs/specification/#top-level-specification-properties) or [vega-lite](https://vega.github.io/vega-lite/docs/spec.html#top-level) schema. The schema URL is an identifier and does not need to be accessible from your browser.

I think the current text is a bit too verbose, and we should stick to our writing principles or being clear and concise.

I am still not happy with the last sentence "The schema URL is an identifier and does not need to be accessible from your browser.", since "browser" sounds like the server could access it, "environment" is imho not a clear enough term.
cc @gchaps Could you help out with phrasing this, that the "URL" will never be "loaded" or tried to access by Kibana or your browser, but is really just an ID in the form of a URL?

@gchaps
Copy link
Contributor

gchaps commented Aug 3, 2020

Does this work?

Your specification requires a $schema field with a valid URL for Vega or Vega-Lite. The URL is an identifier only. Kibana and your browser will never access this URL.

Also, we have specific wording for what experimental means. Do you want to add some wording around that in the message?

This functionality is experimental and is not subject to the support SLA of official GA features. For feedback, please create an issue in Github.

@nyurik
Copy link
Contributor

nyurik commented Aug 3, 2020

I'm not sure we should include the language about the experimental nature of the Vega vis into this (or other) error or warning messages about specific problems.

@alexwizp
Copy link
Contributor Author

alexwizp commented Aug 4, 2020

@elasticmachine merge upstream

@alexwizp
Copy link
Contributor Author

alexwizp commented Aug 4, 2020

thank you @gchaps. An error message was updated
image

@alexwizp
Copy link
Contributor Author

alexwizp commented Aug 5, 2020

@elasticmachine merge upstream

@timroes
Copy link
Contributor

timroes commented Aug 5, 2020

@gchaps the experimental banner is the same for all vis, we can change it to that text, but should do that in a separate PR unrelated to Vega (especially since Vega will anyway be made GA in this release :D)

I've created #74354 for it.

@alexwizp
Copy link
Contributor Author

alexwizp commented Aug 6, 2020

@timroes @stratoula could you please review it?

@timroes timroes removed the release_note:plugin_api_changes Contains a Plugin API changes section for the breaking plugin API changes section. label Aug 6, 2020
@alexwizp
Copy link
Contributor Author

alexwizp commented Aug 7, 2020

@elasticmachine merge upstream

@alexwizp
Copy link
Contributor Author

@elasticmachine merge upstream

if (!spec.$schema) {
throw new Error(
i18n.translate('visTypeVega.vegaParser.inputSpecDoesNotSpecifySchemaErrorMessage', {
defaultMessage: `Your specification requires a {schemaParam} field with a valid URL for
Copy link
Contributor

Choose a reason for hiding this comment

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

Markdown does not seem to be parsed here, so there is no use in using markdown here, we instead should just make this a regular text:

Your specification requires a {schemaParam} field with a valid URL for Vega (see {vegaSchemaUrl}) or Vega-Lite (see {vegaLiteSchemaUrl}). The URL is an identifier only. Kibana and your browser will never access this URL.

Copy link
Contributor

@timroes timroes left a comment

Choose a reason for hiding this comment

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

Left one last comment about getting rid of the Markdown. Otherwise looks good to me.

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Build metrics

async chunks size

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

History

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

@alexwizp alexwizp merged commit 489b134 into elastic:master Aug 11, 2020
alexwizp added a commit to alexwizp/kibana that referenced this pull request Aug 11, 2020
* Deprecate schema-less specs in Vega

Closes elastic#30951

* update an error Message

* update tests

* update error message

* Update vega_parser.ts

Co-authored-by: Elastic Machine <[email protected]>
alexwizp added a commit that referenced this pull request Aug 11, 2020
* Deprecate schema-less specs in Vega

Closes #30951

* update an error Message

* update tests

* update error message

* Update vega_parser.ts

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

Co-authored-by: Elastic Machine <[email protected]>
jloleysens added a commit to jloleysens/kibana that referenced this pull request Aug 12, 2020
…nes/processor-forms-a-d

* 'master' of github.com:elastic/kibana: (25 commits)
  [ML] Removing full lodash library imports (elastic#74742)
  [Search] Server strategy example (elastic#71679)
  [Reporting] Fix and test for Listing of Reports (elastic#74453)
  [maps] fix drawing shapes (elastic#74689)
  [Resolver] Improve simulator. Add more click-through tests and panel tests. (elastic#74601)
  Deprecate schema-less specs in Vega (elastic#73805)
  [Security Solution] Rename Administration > Hosts subtab to Endpoints (elastic#74287)
  Timelion deprecation doc (elastic#74508)
  [Functional Tests] Adds a wait time between setting the index pattern and the time field on TSVB (elastic#74736)
  [Lens] Add styling options for x and y axes on the settings popover (elastic#71829)
  [Maps] add initial location option that fits to data bounds (elastic#74583)
  theme function (elastic#73451)
  [data.ui.query] Write filters to query log from default editor. (elastic#74474)
  [data.search.SearchSource] Move some SearchSource dependencies to the server. (elastic#74607)
  [Canvas][tech-debt] Convert renderers (elastic#74134)
  [security solutions][lists] Adds end to end tests (elastic#74473)
  pluralized for occurrences vs occurrence (elastic#74564)
  Update links that pointed to CONTRIBUTING.md (elastic#74757)
  [Ingest pipelines] Implement tabs in processor flyout (elastic#74469)
  [Event log] Use Alerts client & Actions client when fetching these types of SOs (elastic#73257)
  ...

# Conflicts:
#	x-pack/plugins/ingest_pipelines/public/application/components/pipeline_processors_editor/components/manage_processor_form/field_components/text_editor.tsx
#	x-pack/plugins/ingest_pipelines/public/application/components/pipeline_processors_editor/components/manage_processor_form/manage_processor_form.tsx
#	x-pack/plugins/ingest_pipelines/public/application/components/pipeline_processors_editor/components/manage_processor_form/processors/append.tsx
#	x-pack/plugins/ingest_pipelines/public/application/components/pipeline_processors_editor/components/manage_processor_form/processors/bytes.tsx
#	x-pack/plugins/ingest_pipelines/public/application/components/pipeline_processors_editor/components/manage_processor_form/processors/circle.tsx
#	x-pack/plugins/ingest_pipelines/public/application/components/pipeline_processors_editor/components/manage_processor_form/processors/common_fields/field_name_field.tsx
#	x-pack/plugins/ingest_pipelines/public/application/components/pipeline_processors_editor/components/manage_processor_form/processors/common_fields/ignore_missing_field.tsx
#	x-pack/plugins/ingest_pipelines/public/application/components/pipeline_processors_editor/components/manage_processor_form/processors/convert.tsx
#	x-pack/plugins/ingest_pipelines/public/application/components/pipeline_processors_editor/components/manage_processor_form/processors/csv.tsx
#	x-pack/plugins/ingest_pipelines/public/application/components/pipeline_processors_editor/components/manage_processor_form/processors/date.tsx
#	x-pack/plugins/ingest_pipelines/public/application/components/pipeline_processors_editor/components/manage_processor_form/processors/date_index_name.tsx
#	x-pack/plugins/ingest_pipelines/public/application/components/pipeline_processors_editor/components/manage_processor_form/processors/dissect.tsx
#	x-pack/plugins/ingest_pipelines/public/application/components/pipeline_processors_editor/components/manage_processor_form/processors/dot_expander.tsx
#	x-pack/plugins/ingest_pipelines/public/application/components/pipeline_processors_editor/components/manage_processor_form/processors/drop.tsx
#	x-pack/plugins/ingest_pipelines/public/application/components/pipeline_processors_editor/components/manage_processor_form/processors/index.ts
#	x-pack/plugins/ingest_pipelines/public/application/components/pipeline_processors_editor/components/manage_processor_form/processors/shared.ts
gmmorris added a commit to gmmorris/kibana that referenced this pull request Aug 12, 2020
* master:
  [Vega] add functional tests for Vega visualization (elastic#74097)
  [ML] Removing full lodash library imports (elastic#74742)
  [Search] Server strategy example (elastic#71679)
  [Reporting] Fix and test for Listing of Reports (elastic#74453)
  [maps] fix drawing shapes (elastic#74689)
  [Resolver] Improve simulator. Add more click-through tests and panel tests. (elastic#74601)
  Deprecate schema-less specs in Vega (elastic#73805)
  [Security Solution] Rename Administration > Hosts subtab to Endpoints (elastic#74287)
  Timelion deprecation doc (elastic#74508)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Vega Vega visualizations release_note:breaking 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.

Deprecate schema-less specs in Vega
6 participants