Skip to content

[Usage Collection] [schema] Support spreads + canvas definition#78481

Merged
afharo merged 4 commits intoelastic:masterfrom
afharo:usage_collection/schema/canvas
Sep 29, 2020
Merged

[Usage Collection] [schema] Support spreads + canvas definition#78481
afharo merged 4 commits intoelastic:masterfrom
afharo:usage_collection/schema/canvas

Conversation

@afharo
Copy link
Copy Markdown
Member

@afharo afharo commented Sep 24, 2020 •
edited
Loading

Summary

  • Add schema definition to the collector canvas.
  • Add support to spread definitions in schemas { ...oneSchema, ...otherSchema }.
  • Add support to chasing schema sub-vars in another file.
  • Change the way we enforce Required in MakeSchemaFrom<T> to avoid plugins having to define their schemas as MakeSchemaFrom<Required<MyUsage>>>

Related to #70180.

For maintainers

@afharo afharo added Feature:Telemetry Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas t// v8.0.0 release_note:skip Skip the PR/issue when compiling release notes Team:KibanaTelemetry v7.10.0 labels Sep 24, 2020
@afharo afharo force-pushed the usage_collection/schema/canvas branch 3 times, most recently from 2f5d3f0 to cd2cf9e Compare September 28, 2020 09:21
@afharo afharo force-pushed the usage_collection/schema/canvas branch from cd2cf9e to d570b5e Compare September 28, 2020 09:29
@afharo afharo mentioned this pull request Sep 28, 2020
Closed
14 tasks
@afharo afharo marked this pull request as ready for review September 28, 2020 11:01
@afharo afharo requested review from a team as code owners September 28, 2020 11:01
@elasticmachine
Copy link
Copy Markdown
Contributor

elasticmachine commented Sep 28, 2020

Pinging @elastic/kibana-canvas (Team:Canvas)

@elasticmachine
Copy link
Copy Markdown
Contributor

elasticmachine commented Sep 28, 2020

Pinging @elastic/kibana-telemetry (Team:KibanaTelemetry)

@afharo afharo mentioned this pull request Sep 28, 2020
Merged
1 task
poffdeluxe
poffdeluxe approved these changes Sep 28, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@poffdeluxe poffdeluxe left a comment

Choose a reason for hiding this comment

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

Canvas changes look good to me! Thanks!

TinaHeiligers
TinaHeiligers approved these changes Sep 28, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@TinaHeiligers TinaHeiligers left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for adding the functionality! It will definitely improve the developer experience for cases like the Canvas data.
I have a couple of questions but those are related to the Canvas schema.
Once the merge conflict is resolved, LGTM

x-pack/plugins/canvas/server/collectors/custom_element_collector.ts Outdated
max: { type: 'long' },
avg: { type: 'float' },
},
functions_in_use: { type: 'keyword' },
Copy link
Copy Markdown
Contributor

@TinaHeiligers TinaHeiligers Sep 28, 2020

Choose a reason for hiding this comment

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

should this not be an Array type? The CustomElementTelemetry interface types functions_in_use as string[] and summarizeCustomElement also returns functions_in_use as an array.

Copy link
Copy Markdown
Member Author

@afharo afharo Sep 29, 2020

Choose a reason for hiding this comment

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

You are totally right! I created this PR before the array support was merged. I'll update this and the other comments accordingly! Thank you for catching that up :)

x-pack/plugins/canvas/server/collectors/workpad_collector.ts Outdated
},
functions: {
total: { type: 'long' },
in_use: { type: 'keyword' },
Copy link
Copy Markdown
Contributor

@TinaHeiligers TinaHeiligers Sep 28, 2020

Choose a reason for hiding this comment

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

Why is in_use not defined as an array type in the schema when it is defined as an array in the WorkpadTelemetry interface?

src/fixtures/telemetry_collectors/schema_defined_with_spreads_collector.ts Outdated
isReady: () => true,
fetch() {
const testString = '123';
// query ES and get some data
Copy link
Copy Markdown
Contributor

@TinaHeiligers TinaHeiligers Sep 28, 2020

Choose a reason for hiding this comment

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

These comments aren't necessary. (They probably came along with the copy-paste from the usage_collection README 😉 )

Copy link
Copy Markdown
Member Author

@afharo afharo Sep 29, 2020

Choose a reason for hiding this comment

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

Hawk's eyes! I've simplified this example (no comments, nor try-catch) so it's clear we are testing the spreads, not the fetch implementation.
Thank you for catching that up!

@kibanamachine
Copy link
Copy Markdown
Contributor

kibanamachine commented Sep 29, 2020

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

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

@afharo afharo merged commit 406c47a into elastic:master Sep 29, 2020
@afharo afharo deleted the usage_collection/schema/canvas branch September 29, 2020 11:42
@afharo afharo mentioned this pull request Sep 29, 2020
Merged
afharo added a commit that referenced this pull request Sep 29, 2020
@afharo
[7.x] [Usage Collection] [schema] Support spreads + canvas definiti…
60763bd 
…on (#78481) (#78757)
@afharo afharo linked an issue Sep 29, 2020 that may be closed by this pull request
[Canvas][Telemetry] Add schema to canvas collector #75746
Closed
@afharo afharo mentioned this pull request Sep 29, 2020
Closed
phillipb added a commit to phillipb/kibana that referenced this pull request Sep 29, 2020
@phillipb
Merge branch 'master' of github.com:elastic/kibana into add-anomalies…
f7e5f99 
…-to-timeline

* 'master' of github.com:elastic/kibana: (22 commits)
  update apm index pattern (elastic#78732)
  78024: move transform out of dataset (elastic#78216)
  [QA][Code Coverage] Upload the coverage static site before ingestion (elastic#78695)
  [Discover] Make _source field not clickable (elastic#78698)
  [Fleet] Rename Ingest Manager => Fleet, Fleet => Agents in the UI (elastic#78685)
  [APM] Review feedback from distribution + transaction metrics (elastic#78752)
  [Ingest pipelines] Add ability to stop pipeline simulation  (elastic#78183)
  [CSM] Fix core vital legend background (elastic#78273)
  [Usage Collection] [schema] Support spreads + `canvas` definition (elastic#78481)
  fix lodash imports (elastic#78456)
  [Maps] Add layer type preview icons (elastic#78650)
  [APM] Use transaction metrics for distribution charts (elastic#78484)
  [Uptime] Ml anomaly alert edit (elastic#76909)
  [ML] Limit exposing shared static code through ml/public/index.ts. (elastic#77745)
  making expression debug info serializable (elastic#78727)
  fix lodahs imports in app-arch code (elastic#78582)
  Make Field a React.lazy export (elastic#78483)
  [Security Solution] Improves detections tests (elastic#77295)
  [TSVB] Different field format on different series is ignored (elastic#78138)
  RFC: Improve saved object migrations (elastic#66056)
  ...
@lukeelmers lukeelmers added the Team:Core Platform Core services: plugins, logging, config, saved objects, http, ES client, i18n, etc t// label Oct 1, 2021
@elasticmachine
Copy link
Copy Markdown
Contributor

elasticmachine commented Oct 1, 2021

Pinging @elastic/kibana-core (Team:Core)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@TinaHeiligers TinaHeiligers TinaHeiligers approved these changes

+1 more reviewer

@poffdeluxe poffdeluxe poffdeluxe approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Feature:Telemetry release_note:skip Skip the PR/issue when compiling release notes Team:Core Platform Core services: plugins, logging, config, saved objects, http, ES client, i18n, etc t// Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas t// v7.10.0 v8.0.0

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Canvas][Telemetry] Add schema to canvas collector

6 participants

@afharo @elasticmachine @kibanamachine @poffdeluxe @TinaHeiligers @lukeelmers