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.

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

Open
wants to merge 29 commits into
base: main
Choose a base branch
from

Conversation

Bamieh
Copy link
Member

@Bamieh Bamieh commented Oct 14, 2024

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

  1. Core service to flag deprecated routes
  2. UA code to surface and resolve deprecated routes

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

router[versioned?].get(
    {
      path: '/',
      options: {
        deprecated: {
           documentationUrl: 'https://google.com',
           severity: 'warning',
           reason: {
              type: 'bump',
              newApiVersion: '2024-10-13',
            },
        },
      },
    },
    async (context, req, res) => {
...

Surfaced API deprecations in UA

The list of deprecated APIs will be listed inside Kibana deprecations along with the already supported config deprecations.
image

Users can click on the list item to learn more about each deprecation and mark it as resolved
image

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:

The API GET /api/deprecations/ has been called 25 times. The last time the API was called was on Monday, October 14, 2024 1:08 PM +03:00.
The api has been called 2 times since the last time it was marked as resolved on Monday, October 14, 2024 1:08 PM +03:00

Once marked as resolved the flyout exists and we show this to the user until they refresh the page
image

Telemetry:

We keep track of 2 new things for telemetry purposes:

  1. The number of times the deprecated API has been called
  2. The number of times the deprecated API has been resolved (how many times the mark as resolved button in UA was clicked)

Code review

  • Core team is expected to review the whole PR
  • kibana-management team is expected to review the UA code changes and UI
  • A few teams are only required to approve this PR and update their 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 replace deprecated: true with the deprecated object #196095

Testing

Run kibana locally with the test example plugin that has deprecated routes

yarn start --plugin-path=examples/routing_example --plugin-path=examples/developer_examples

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:

# Versioned routes: Version 1 is deprecated
GET kbn:/api/routing_example/d/versioned?apiVersion=1
GET kbn:/api/routing_example/d/versioned?apiVersion=2

# Non-versioned routes
GET kbn:/api/routing_example/d/removed_route
POST kbn:/api/routing_example/d/migrated_route
{}
  1. You can also mark as deprecated in the UA to remove the deprecation from the list.
  2. Check the telemetry response to see the reported data about the deprecated route.
  3. Calling version 2 of the API does not do anything since it is not deprecated unlike version 1 (GET kbn:/api/routing_example/d/versioned?apiVersion=2)
  4. Internally you can see the deprecations counters from the dev console by running the following:
GET .kibana_usage_counters/_search
{
    "query": {
        "bool": {
            "should": [
              {"match": { "usage-counter.counterType": "deprecated_api_call:total"}},
              {"match": { "usage-counter.counterType": "deprecated_api_call:resolved"}},
              {"match": { "usage-counter.counterType": "deprecated_api_call:marked_as_resolved"}}
            ]
        }
    }
}

@Bamieh Bamieh requested review from a team as code owners October 14, 2024 10:15
@Bamieh Bamieh added 8.16 candidate release_note:skip Skip the PR/issue when compiling release notes labels Oct 14, 2024
@Bamieh Bamieh mentioned this pull request Oct 14, 2024
@Bamieh Bamieh added the backport:skip This commit does not require backporting label Oct 14, 2024
@Bamieh Bamieh changed the title finish feature [Core] [UA] Support API Deprecations Oct 14, 2024
Copy link
Contributor

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

screenshot Screenshot 2024-10-14 at 15 19 20

@jloleysens
Copy link
Contributor

Also, I attempted to mark an unversioned API deprecation as resolved and got the following:

{
    "statusCode": 400,
    "error": "Bad Request",
    "message": "[request body.routeVersion]: expected value of type [string] but got [undefined]"
}

Copy link
Contributor

@jloleysens jloleysens left a 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:

  1. 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!
  2. A callout (warning) for when a resolved deprecation gets re-raised

Would be nice to get copy review for the corrective actions too!

screenshot Screenshot 2024-10-15 at 13 56 44

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

screenshot

Bottom two deprecations lost their "Marked as resolved" status.

Screenshot 2024-10-15 at 13 56 59

Copy link
Contributor

@jloleysens jloleysens left a 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

Copy link
Contributor

@TinaHeiligers TinaHeiligers left a 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

@Bamieh Bamieh requested review from a team as code owners October 17, 2024 15:12
@botelastic botelastic bot added the Team:Fleet Team label for Observability Data Collection Fleet team label Oct 17, 2024
@elasticmachine
Copy link
Contributor

Pinging @elastic/fleet (Team:Fleet)

Copy link
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
FYI we are planning to remove these deprecated APIs in 9.0: https://github.com/elastic/dev/issues/2818

Copy link
Contributor

@ElenaStoeva ElenaStoeva left a comment

Choose a reason for hiding this comment

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

Thanks for working on this @Bamieh! Regarding testing these changes in UA, can you add testing instructions, including how to create the deprecation from the screenshots?

Also, could you add/update the jest integration tests for the changed components in UA?

@TinaHeiligers
Copy link
Contributor

Regarding testing these changes in UA, can you add testing instructions, including how to create the deprecation from the screenshots?

@ElenaStoeva The steps for testing the Upgrade Assistant are in the readme. I don't know if they'll cover this work but it's a place to start.

Bamieh and others added 5 commits October 20, 2024 13:58
…t --include-path /api/status --include-path /api/alerting/rule/ --include-path /api/alerting/rules --include-path /api/actions --include-path /api/security/role --include-path /api/spaces --include-path /api/fleet --update'
@Bamieh
Copy link
Member Author

Bamieh commented Oct 20, 2024

@ElenaStoeva I added a testing section to the PR comment #196081 (comment)

I created an example plugin that we can use for testing, please check the instructions to follow through with testing

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
@kbn/core-http-server 216 232 +16
@kbn/core-usage-data-server 144 154 +10
total +26

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
upgradeAssistant 157.2KB 158.8KB +1.6KB

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

id before after diff
@kbn/core-deprecations-common 0 1 +1
@kbn/core-http-server 0 1 +1
total +2

Page load bundle

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

id before after diff
core 450.0KB 450.5KB +519.0B
Unknown metric groups

API count

id before after diff
@kbn/core-http-server 532 550 +18
@kbn/core-usage-data-server 155 165 +10
total +28

ESLint disabled in files

id before after diff
@kbn/core-deprecations-server-internal 2 1 -1

Total ESLint disabled count

id before after diff
@kbn/core-deprecations-server-internal 2 1 -1

History

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
8.16 candidate backport:skip This commit does not require backporting release_note:skip Skip the PR/issue when compiling release notes Team:Fleet Team label for Observability Data Collection Fleet team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support Kibana deprecation logging in Upgrade Assistant
8 participants