Skip to content

[Telemetry] Use header-based versioned APIs instead of path-based#159839

Merged
afharo merged 9 commits intoelastic:mainfrom
afharo:telemetry/use-versioned-routing
Aug 12, 2023
Merged

[Telemetry] Use header-based versioned APIs instead of path-based#159839
afharo merged 9 commits intoelastic:mainfrom
afharo:telemetry/use-versioned-routing

Conversation

@afharo
Copy link
Copy Markdown
Member

@afharo afharo commented Jun 15, 2023 •
edited
Loading

Summary

Related to #159181.

It moves the current telemetry endpoints from path-based to header-based versioning. It also flags them as internal for Serverless.

The only one kept for BWC is GET /api/telemetry/v2/config, as we are aware of some consumers of this API.

Checklist

Risk Matrix

Risk Probability Severity Mitigation/Notes
Some consumers might stop working. Low Low These APIs have always been treated as internal and shouldn't be used outside of Kibana (apart from the config endpoint).

For maintainers

@afharo afharo added Feature:http release_note:breaking Team:Core Platform Core services: plugins, logging, config, saved objects, http, ES client, i18n, etc t// Feature:Telemetry backport:skip This PR does not require backporting Project:Serverless Work as part of the Serverless project for its initial release labels Jun 15, 2023
@afharo afharo self-assigned this Jun 15, 2023
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Jun 15, 2023

Documentation preview:

afharo
afharo commented Aug 7, 2023
View reviewed changes
x-pack/dev-tools/api_debug/apis/telemetry/index.js
export const description = 'Get the clusters stats from the Kibana server';
export const method = 'POST';
export const path = '/api/telemetry/v2/clusters/_stats';
export const path = '/internal/telemetry/clusters/_stats';
Copy link
Copy Markdown
Member Author

@afharo afharo Aug 7, 2023

Choose a reason for hiding this comment

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

I noticed this script already supports the internal header but doesn't support the version header.

We might need to solve it in a follow-up PR.

@afharo afharo marked this pull request as ready for review August 7, 2023 16:05
@afharo afharo requested review from a team as code owners August 7, 2023 16:05
@elasticmachine
Copy link
Copy Markdown
Contributor

elasticmachine commented Aug 7, 2023

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

@afharo afharo mentioned this pull request Jun 16, 2023
Open
nreese
nreese approved these changes Aug 7, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@nreese nreese left a comment

Choose a reason for hiding this comment

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

kibana_gis changes LGTM

@botelastic botelastic bot added the Team:Fleet Team label for Observability Data Collection Fleet team label Aug 7, 2023
@elasticmachine
Copy link
Copy Markdown
Contributor

elasticmachine commented Aug 7, 2023

Pinging @elastic/fleet (Team:Fleet)

juliaElastic
juliaElastic approved these changes Aug 8, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@juliaElastic juliaElastic left a comment

Choose a reason for hiding this comment

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

Fleet change LGTM

jloleysens
jloleysens approved these changes Aug 8, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@jloleysens jloleysens 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.

Core changes LGTM, makes sense to maintain public routes and register duplicate internal ones. In the future we will be free to make v3, v4 but will need to maintain 2023-10-31 for the time being.

src/plugins/telemetry/public/services/telemetry_service.ts
Comment on lines +191 to +192
return this.http.post(FetchSnapshotTelemetry, {
...INTERNAL_VERSION,
Copy link
Copy Markdown
Contributor

@jloleysens jloleysens Aug 8, 2023

Choose a reason for hiding this comment

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

This code looks good, but it does make me wonder whether we might consider providing a away to make a "version scoped" client so that you'd have smth like:

const httpV1 = this.http.createScopedClient({ version: '1' })
...
httpV1.post(FetchSnapshotTelemetry...)

but yeah, just a thought, nothing to action right now.

Copy link
Copy Markdown
Member Author

@afharo afharo Aug 8, 2023

Choose a reason for hiding this comment

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

That's a great idea! I would ask contributors if they'd use it.

@afharo afharo added Team:Infra Monitoring UI - DEPRECATED DEPRECATED - Label for the Infra Monitoring UI team. Use Team:obs-ux-infra_services Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. Team:SharedUX Platform AppEx-SharedUX (formerly Global Experience) t// labels Aug 11, 2023
@elasticmachine
Copy link
Copy Markdown
Contributor

elasticmachine commented Aug 11, 2023

Pinging @elastic/infra-monitoring-ui (Team:Infra Monitoring UI)

@elasticmachine
Copy link
Copy Markdown
Contributor

elasticmachine commented Aug 11, 2023

Pinging @elastic/appex-sharedux (Team:SharedUX)

@elasticmachine
Copy link
Copy Markdown
Contributor

elasticmachine commented Aug 11, 2023

Pinging @elastic/security-solution (Team: SecuritySolution)

tonyghiani
tonyghiani approved these changes Aug 11, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@tonyghiani tonyghiani left a comment

Choose a reason for hiding this comment

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

Infra change LGTM

banderror
banderror approved these changes Aug 11, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@banderror banderror left a comment

Choose a reason for hiding this comment

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

Security Solution changes LGTM 👍

@afharo
Copy link
Copy Markdown
Member Author

afharo commented Aug 12, 2023

@elasticmachine merge upstream

@afharo afharo enabled auto-merge (squash) August 12, 2023 20:15
@kibana-ci
Copy link
Copy Markdown

kibana-ci commented Aug 12, 2023

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
telemetry 30 31 +1

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
telemetry 19.2KB 19.2KB -15.0B

History

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

cc @afharo

@afharo afharo merged commit 0284cc1 into elastic:main Aug 12, 2023
@afharo afharo deleted the telemetry/use-versioned-routing branch August 12, 2023 21:20
jughosta pushed a commit to jughosta/kibana that referenced this pull request Aug 14, 2023
@afharo @jughosta
[Telemetry] Use header-based versioned APIs instead of path-based (el…
f0dfe07 
…astic#159839)
@rhr323 rhr323 mentioned this pull request Sep 6, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nreese nreese nreese approved these changes

@jbudz jbudz jbudz approved these changes

@banderror banderror banderror approved these changes

@Dosant Dosant Dosant approved these changes

@jloleysens jloleysens jloleysens approved these changes

@tonyghiani tonyghiani tonyghiani approved these changes

@juliaElastic juliaElastic juliaElastic approved these changes

Assignees

@afharo afharo

Labels

backport:skip This PR does not require backporting Feature:http Feature:Telemetry Project:Serverless Work as part of the Serverless project for its initial release release_note:breaking Team:Core Platform Core services: plugins, logging, config, saved objects, http, ES client, i18n, etc t// Team:Fleet Team label for Observability Data Collection Fleet team Team:Infra Monitoring UI - DEPRECATED DEPRECATED - Label for the Infra Monitoring UI team. Use Team:obs-ux-infra_services Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. Team:SharedUX Platform AppEx-SharedUX (formerly Global Experience) t// v8.10.0

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.