-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[Core] [UA] Support API Deprecations #196081
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still busy reviewing, but overall this is looking great @Bamieh
just leaving a few initial observations based on usage.
Looks like interpolation is not happening as expected on step 3.
...es/core/deprecations/core-deprecations-server-internal/src/routes/post_validation_handler.ts
Outdated
Show resolved
Hide resolved
packages/core/http/core-http-server-internal/src/http_server.ts
Outdated
Show resolved
Hide resolved
Also, I attempted to mark an unversioned API deprecation as resolved and got the following:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for addressing my feedback @Bamieh ! I left mostly nits in the code. The main missing piece right now is just some strategic placed integration tests. I think unit-level tests would be nice to have for sure, but I'll leave that up to you.
I think we should get copy review but I had one round of feedback for the structure of the first couple paragrahs of text in the flyout. I think we should surface the most important bits:
- When was this API last called (I think this is the most important bit of info). We could also mention that if a deprecation is marked as resolved we will re-raise so users should check back!
- A callout (warning) for when a resolved deprecation gets re-raised
Would be nice to get copy review for the corrective actions too!
I really like the flow of marking API deprecations as resolved. It would be nice if the "marked resolved" indicator remains visible. I went through 3 deprecations and after marking each as resolved the deprecation would remain visible (which was nice), but the "marked as resolved" indicator would vanish
...ges/core/deprecations/core-deprecations-server-internal/src/routes/resolve_deprecated_api.ts
Outdated
Show resolved
Hide resolved
packages/core/http/core-http-router-server-internal/src/router.ts
Outdated
Show resolved
Hide resolved
...ages/core/http/core-http-router-server-internal/src/versioned_router/core_versioned_route.ts
Show resolved
Hide resolved
packages/core/http/core-http-router-server-internal/src/versioned_router/types.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To unblock progress I am pre-approving pending:
- tests
- copy review
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The implementation looks good code-wise, I haven't tested it.
I've left a few comments and nits, including type-issues that you may not be aware of.
LGTM on CI green
src/plugins/kibana_usage_collection/server/collectors/core/fetch_deprecated_api_counters.ts
Outdated
Show resolved
Hide resolved
src/plugins/kibana_usage_collection/server/collectors/core/fetch_deprecated_api_counters.ts
Outdated
Show resolved
Hide resolved
src/plugins/kibana_usage_collection/server/collectors/core/fetch_deprecated_api_counters.ts
Outdated
Show resolved
Hide resolved
packages/core/usage-data/core-usage-data-server-internal/src/core_usage_stats_client.ts
Outdated
Show resolved
Hide resolved
packages/core/usage-data/core-usage-data-server-internal/src/core_usage_stats_client.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Defend Workflows code changes review LGTM 👍
x-pack/plugins/security_solution/server/endpoint/routes/metadata/index.ts
Outdated
Show resolved
Hide resolved
x-pack/plugins/security_solution/server/endpoint/routes/suggestions/index.ts
Outdated
Show resolved
Hide resolved
… core/api-deprecations
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DW changes LGTM, thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ML changes LGTM
@elasticmachine merge upstream |
💚 Build Succeeded
Metrics [docs]Public APIs missing comments
Async chunks
Public APIs missing exports
Page load bundle
Unknown metric groupsAPI count
ESLint disabled in files
Total ESLint disabled count
History
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested UA UI again and this time it works as expected! Changes lgtm. 👍
Summary
Adds a new API deprecations feature inside core.
This feature enabled plugin developers to mark their versioned and unversioned public routes as deprecated.
These deprecations will be surfaced to the users through UA to help them understand the deprecation and address it before upgrading. This PR also surfaces these deprecations to UA.
Closes #117241
Flagging a deprecated Route
The route deprecation option
We have three types of route deprecations:
type: bump
: A version bump deprecation means the API has a new version and the current version will be removed in the future in favor of the newer version.type: remove
: This API will be completely removed. You will no longer be able to use it in the future.type: migrate
: This API will be migrated to a different API and will be removed in the future in favor of the other API.All route deprecations expect a documentation link to help users navigate. We might add a generic documentation link and drop this requirement in the future but for now this is required.
Deprecated Route Example
Full examples can be found in the
routing_example
example plugin located in this directory:examples/routing_example/server/routes/deprecated_routes
Surfaced API deprecations in UA
The list of deprecated APIs will be listed inside Kibana deprecations along with the already supported config deprecations.
Users can click on the list item to learn more about each deprecation and mark it as resolved
Marking as resolved
Users can click on mark as resolved button in the UA to hide the deprecation from the Kiban deprecations list.
We keep track on when this button was clicked and how many times the API has been called. If the API is called again the deprecation will re-appear inside the list. We might add a feature in the future to permenantly supress the API deprecation from showing in the list through a configuration (#196089)
If the API has been marked as resolved before we show this in the flyout message:
Once marked as resolved the flyout exists and we show this to the user until they refresh the page
Telemetry:
We keep track of 2 new things for telemetry purposes:
Code review
deprecated: true
route param to the new deprecationInfo object we now expect. There is an issue tracker to address those in separate PRs later on: API Deprecations replacedeprecated: true
with the deprecated object #196095Testing
Run kibana locally with the test example plugin that has deprecated routes
The following comprehensive deprecated routes examples are registered inside the folder:
examples/routing_example/server/routes/deprecated_routes
Run them in the console to trigger the deprecation condition so they show up in the UA:
1
(GET kbn:/api/routing_example/d/versioned?apiVersion=2
)