[Telemetry] Full `schema` definition by afharo · Pull Request #90273 · elastic/kibana

afharo · 2021-02-04T11:54:01Z

Summary

This PR adds the schema definitions to the full payloads sent from the OSS and X-Pack local collectors.

It also creates some functional tests that validate that the schemas are up-to-date: for now, it primarily checks that we don't report fields that are not detailed in the schema. But it intentionally defines all the fields as optional. We can create additional tests in the future to ensure certain keys are mandatory.

Example of the error thrown during CI:

     │1)    apis
     │       Telemetry
     │         /api/telemetry/v2/clusters/_stats with monitoring disabled
     │           should pass the schema validation:
     │
     │      Error: The telemetry schemas in 'x-pack/plugins/telemetry_collection_xpack/schema/' are out-of-date, please update it as required: [stack_stats.kibana.plugins.spaces.disabledFeatures.savedObjectsTagging]: definition for this key is missing. Received `0`

Last, but not least, it deletes the legacy_oss definition.

TODO: Planned for a follow-up issue: define the monitoring payload (#90795).

Resolves #83704

Code Owners Review

Why am I required to review this? If your team has been asked to review, it is quite likely that we've found some issues when running the functional tests. I've tried to fix it the way I think it makes more sense to me, but please, let me know if a different approach should be taken in your collector.

Checklist

Delete any items that are not applicable to this PR.

Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios

For maintainers

This was checked for breaking API changes and was labeled appropriately

afharo

self-review/comments

test/api_integration/apis/telemetry/telemetry_local.ts

x-pack/plugins/actions/server/usage/actions_usage_collector.ts

x-pack/plugins/alerts/server/usage/alerts_usage_collector.ts

x-pack/plugins/alerts/server/usage/alerts_telemetry.ts

x-pack/plugins/spaces/server/usage_collection/spaces_usage_collector.ts

x-pack/plugins/telemetry_collection_xpack/schema/xpack_root.json

afharo · 2021-02-05T16:11:47Z

x-pack/plugins/telemetry_collection_xpack/schema/xpack_root.json

+                            }
+                          }
+                        },
+                        "DYNAMIC_KEY": {


This comment applies to all the usages of DYNAMIC_KEY int he *_root.json files. Should we be more explicit in these, since we should know in advance all the possible values?

cc/ @elastic/infra-telemetry

Should we be more explicit in these, since we should know in advance all the possible values?

+1 for explicit values, if we know them in advance 🙏

I'd like some input from the ES team here to confirm if we'll know them in advance

elasticmachine · 2021-02-05T18:49:31Z

Pinging @elastic/apm-ui (Team:apm)

YulNaumenko

Alerting related changes LGTM.

x-pack/plugins/actions/server/usage/actions_usage_collector.ts

afharo · 2021-02-08T09:50:52Z

@elasticmachine merge upstream

pgayvallet

LGTM

test/api_integration/apis/telemetry/telemetry_local.ts

test/api_integration/apis/telemetry/utils/schema_to_config_schema.ts

Bamieh

Code LGTM.

I just want to voice my concerns here for the sake of clarity; As I stated earlier am not super happy baking these tests into our code base (#83704 (comment)). I am ok with merging this since the team feels OK doing that however.

I still believe we should not be having the full schema definition in kibana. We are basically adding tests for elasticsearch in our kibana codebase.

I'm worried that before every release all this will be skipped and marked as a flaky test.
The build priority is higher than fixing telemetry tests that fail from an ES update. This means the burden of updating those will always fall on our shoulders now.

The telemetry_check will not fix the breaking changes in the root schemas (and cannot since there was a lot of manual checking involved to writing those root schemas).

x-pack/plugins/telemetry_collection_xpack/schema/xpack_plugins.json

afharo · 2021-02-09T10:14:46Z

@elasticmachine merge upstream

afharo · 2021-02-15T09:31:47Z

Adding @rjernst as a reviewer because we are waiting for the input from the ES team because they'll have to maintain the possible changes coming from the ES APIs passed through.

Related issue elastic/elasticsearch#58198

TinaHeiligers

Adding an LGTM to unblock a review needed from the kibana-telemetry team

afharo · 2021-02-17T11:53:27Z

@elasticmachine merge upstream

Sorry for the noise, just keeping this up-to-date to test how often will this break in the future as other teams push features and changes :)

afharo · 2021-02-19T11:27:26Z

@elasticmachine merge upstream

Sorry for the noise (again), just keeping this up-to-date to test how often will this break in the future as other teams push features and changes :)

afharo · 2021-02-22T10:13:48Z

@elasticmachine merge upstream

Sorry for the noise (again), just keeping this up-to-date to test how often will this break in the future as other teams push features and changes :)

…l-schema-definition

afharo · 2021-02-25T17:54:45Z

In order to unblock some goodness this PR brings in (especially FTRs to make sure what we actually report matches what we claim in the schema), I've created the pass_through type so we can validate them as schema.any(). It is applied to all the fields we simply pass-through as they are from ES.

#92867 will add the definition of ES fields once we agree on the best way to keep them up-to-sync.

afharo · 2021-03-01T11:53:40Z

@elasticmachine merge upstream

kibanamachine · 2021-03-01T17:38:42Z

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

💚 Build #109860 succeeded 74967d0
💚 Build #109506 succeeded f041b19
💚 Build #108305 succeeded 2d80f23
💚 Build #107965 succeeded 7b8096c
💚 Build #107239 succeeded a4d2e8b

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

TinaHeiligers

The changes since I last reviewed the PR look good to me.
My second review (this one) is a code-review only.
LGTM!

kibanamachine · 2021-03-01T18:34:21Z

💔 Backport failed

❌ 7.x: Commit could not be cherrypicked due to conflicts

To backport manually, check out the target branch and run:
node scripts/backport --pr 90273

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> # Conflicts: # test/api_integration/apis/telemetry/telemetry_local.ts

* [Telemetry] Full `schema` definition (#90273) Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> # Conflicts: # test/api_integration/apis/telemetry/telemetry_local.ts * Normalize test arguments with `master` * Normalize to `master` the `/api/settings` FTR tests

afharo added release_note:skip Skip the PR/issue when compiling release notes v7.12.0 labels Feb 4, 2021

afharo added 6 commits February 5, 2021 12:23

[Telemetry] Full schema definition

1b529a1

Add OSS tests

2cf5fed

Sort OSS tests

9e00b1b

Add X-Pack tests + better message error message

d2c0bd6

Fix missing fields in xpack_root

b28e262

Fix schemas for actions, alerts and APM

1e17622

afharo force-pushed the telemetry/full-schema-definition branch from 65d7788 to 1e17622 Compare February 5, 2021 15:49

Add documentation about testing

c3a9418

afharo commented Feb 5, 2021

View reviewed changes

Update APM test snapshot

fdd6f19

afharo marked this pull request as ready for review February 5, 2021 18:33

afharo requested a review from a team February 5, 2021 18:33

afharo requested review from a team as code owners February 5, 2021 18:33

botelastic bot added the Team:APM - DEPRECATED Use Team:obs-ux-infra_services. label Feb 5, 2021

YulNaumenko approved these changes Feb 5, 2021

View reviewed changes

x-pack/plugins/actions/server/usage/actions_usage_collector.ts Show resolved Hide resolved

kibanamachine and others added 2 commits February 8, 2021 04:50

Merge branch 'master' into telemetry/full-schema-definition

42e8489

Use DYNAMIC_KEY in the spaces.disabledFeatures collector

6705e90

pgayvallet approved these changes Feb 8, 2021

View reviewed changes

test/api_integration/apis/telemetry/telemetry_local.ts Show resolved Hide resolved

test/api_integration/apis/telemetry/utils/schema_to_config_schema.ts Show resolved Hide resolved

Fix xpack.data_tiers schema

64102c9

Bamieh approved these changes Feb 9, 2021

View reviewed changes

x-pack/plugins/telemetry_collection_xpack/schema/xpack_plugins.json Show resolved Hide resolved

Merge branch 'master' into telemetry/full-schema-definition

4e2e0bb

This was referenced Feb 9, 2021

[discuss] Usage Collection: Should we use config-schema instead of our schema implementation? #85810

Closed

We should have a dedicated endpoint for telemetry elastic/elasticsearch#58198

Open

Merge branch 'master' into telemetry/full-schema-definition

4f9e3e7

afharo requested a review from rjernst February 15, 2021 09:29

TinaHeiligers approved these changes Feb 15, 2021

View reviewed changes

Merge branch 'master' into telemetry/full-schema-definition

a4d2e8b

Merge branch 'master' into telemetry/full-schema-definition

7b8096c

kibanamachine and others added 3 commits February 22, 2021 05:13

Merge branch 'master' into telemetry/full-schema-definition

2d80f23

Merge branch 'master' of github.com:elastic/kibana into telemetry/ful…

a0a80e8

…l-schema-definition

Do not validate ES-sourced data for now

f041b19

afharo mentioned this pull request Feb 25, 2021

[Telemetry] Full schema including Elasticsearch fields #92867

Closed

9 tasks

afharo requested a review from a team February 25, 2021 17:54

kibanamachine and others added 2 commits March 1, 2021 06:53

Merge branch 'master' into telemetry/full-schema-definition

74967d0

Add documentation about the new type pass_through

eef6173

afharo requested a review from mindbat March 1, 2021 17:25

TinaHeiligers approved these changes Mar 1, 2021

View reviewed changes

afharo added auto-backport Deprecated - use backport:version if exact versions are needed v7.13.0 and removed v7.12.0 labels Mar 1, 2021

afharo merged commit f44916b into elastic:master Mar 1, 2021

afharo deleted the telemetry/full-schema-definition branch March 1, 2021 18:30

afharo mentioned this pull request Mar 2, 2021

[7.x] [Telemetry] Full schema definition (#90273) #93187

Merged

Conversation

afharo commented Feb 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Code Owners Review

Checklist

For maintainers

Uh oh!

afharo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

afharo Feb 5, 2021

Choose a reason for hiding this comment

Uh oh!

afharo Feb 8, 2021

Choose a reason for hiding this comment

Uh oh!

mindbat Feb 9, 2021

Choose a reason for hiding this comment

Uh oh!

afharo Feb 11, 2021

Choose a reason for hiding this comment

Uh oh!

elasticmachine commented Feb 5, 2021

Uh oh!

YulNaumenko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

afharo commented Feb 8, 2021

Uh oh!

pgayvallet left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Bamieh left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

afharo commented Feb 9, 2021

Uh oh!

afharo commented Feb 15, 2021

Uh oh!

TinaHeiligers left a comment

Choose a reason for hiding this comment

Uh oh!

afharo commented Feb 17, 2021

Uh oh!

afharo commented Feb 19, 2021

Uh oh!

afharo commented Feb 22, 2021

Uh oh!

afharo commented Feb 25, 2021

Uh oh!

afharo commented Mar 1, 2021

Uh oh!

kibanamachine commented Mar 1, 2021

💚 Build Succeeded

Metrics [docs]

History

Uh oh!

TinaHeiligers left a comment

Choose a reason for hiding this comment

Uh oh!

kibanamachine commented Mar 1, 2021

💔 Backport failed

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

afharo commented Feb 4, 2021 •

edited

Loading

Bamieh left a comment •

edited

Loading