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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 4 commits into from
Sep 29, 2020
Merged

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

merged 4 commits into from
Sep 29, 2020

Conversation

afharo
Copy link
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 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
Contributor

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

@elasticmachine
Copy link
Contributor

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
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
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
Contributor

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
Member Author

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
Contributor

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
Contributor

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
Member Author

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
Contributor

💚 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 Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc label Oct 1, 2021
@elasticmachine
Copy link
Contributor

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

@poffdeluxe poffdeluxe poffdeluxe approved these changes

Assignees
No one assigned
Labels
Feature:Telemetry release_note:skip Skip the PR/issue when compiling release notes Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas 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