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

Short URLs #107859

Merged
merged 18 commits into from
Sep 28, 2021
Merged

Short URLs #107859

merged 18 commits into from
Sep 28, 2021

Conversation

vadimkibana
Copy link
Contributor

@vadimkibana vadimkibana commented Aug 8, 2021

Summary

Closes #98110
Closes #93047

Implements new Short URL service.

Rest API

Create new short URL

POST /api/short_url

  • locatorId - ID of the locator which to use to generate resolved URL.
  • params - an object of locator params.
  • slug - (optional) human readable URL slug, used to identify the short URL.
  • humanReadableSlug - (optional) if true, a human readable slug will be generated on the server.
Delete a short URL

Deletes an existing short URL.

DELETE /api/short_url/{id}

  • {id} - ID of the short URL.
Get a short URL

Fetches a short URL by its ID.

Get /api/short_url/{id}

  • {id} - ID of the short URL.
Resolve a short URL

Fetches a short URL by its slug.

Get /api/short_url/_slug/{slug}

  • {slug} - a human-readable slug of the short URL.

Checklist

Delete any items that are not applicable to this PR.

Risk Matrix

Delete this section if it is not applicable to this PR.

Before closing this PR, invite QA, stakeholders, and other developers to identify risks that should be tested prior to the change/feature release.

When forming the risk matrix, consider some of the following examples and how they may potentially impact the change:

Risk Probability Severity Mitigation/Notes
Old short URLs might stop working. Low High An API integration test was added to check legacy short URLs still work.

For maintainers


Release note

Short URL service was updated to support short URL creation from locators. Short URL service now supports human readable slugs for short URLs.

@vadimkibana vadimkibana marked this pull request as ready for review September 7, 2021 12:11
@vadimkibana vadimkibana requested review from a team as code owners September 7, 2021 12:11
Copy link
Contributor

@peteharverson peteharverson left a comment

Choose a reason for hiding this comment

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

ML edit LGTM

@botelastic botelastic bot added the Team:APM All issues that need APM UI Team support label Sep 7, 2021
@elasticmachine
Copy link
Contributor

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

Copy link
Contributor

@alisonelizabeth alisonelizabeth left a comment

Choose a reason for hiding this comment

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

Stack Management changes LGTM - one-line change to ingest node pipelines locator test.

Copy link
Member

@kertal kertal left a comment

Choose a reason for hiding this comment

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

DataDiscovery team owned code LGTM, just mocks and tests were modified. So I didn't test locally. Would just suggest to add a bit more context to this PR's description.

Copy link
Contributor

@smith smith left a comment

Choose a reason for hiding this comment

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

APM changes look good.

Copy link
Contributor

@stratoula stratoula left a comment

Choose a reason for hiding this comment

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

Kibana vis editors code changes LGTM! It is just a change in a test file so I just did a code review :)


/**
* Common URL Service client interface for server-side and client-side.
*/
export class UrlService {
export class UrlService<D = unknown> {
Copy link
Member

Choose a reason for hiding this comment

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

why do we need generic here ? seems we know deps.shortUrls needs to be there and we also know all deps for LocatorClient ? also having D being unknown by default is a bit weird and not fully type safe.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

why do we need generic here ? seems we know deps.shortUrls needs to be there and we also know all deps for LocatorClient ?

It is generic because Short URL service has different dependencies on the server vs on the browser.

also having D being unknown by default is a bit weird and not fully type safe.

Why do you think it is weird and not safe?

@vadimkibana vadimkibana added release_note:feature Makes this part of the condensed release notes Team:AppServices v7.16.0 v8.0.0 and removed Team:APM All issues that need APM UI Team support labels Sep 20, 2021
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app-services (Team:AppServices)

@vadimkibana vadimkibana added the auto-backport Deprecated - use backport:version if exact versions are needed label Sep 20, 2021
@botelastic botelastic bot added the Team:APM All issues that need APM UI Team support label Sep 20, 2021
@vadimkibana
Copy link
Contributor Author

@elasticmachine merge upstream

@vadimkibana
Copy link
Contributor Author

@elasticmachine merge upstream

@vadimkibana
Copy link
Contributor Author

@elasticmachine merge upstream

@vadimkibana
Copy link
Contributor Author

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💛 Build succeeded, but was flaky

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
share 67 71 +4

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
share 88 90 +2

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
share 9 10 +1

Page load bundle

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

id before after diff
share 55.9KB 57.8KB +1.8KB

Saved Objects .kibana field count

Every field in each saved object type adds overhead to Elasticsearch. Kibana needs to keep the total field count below Elasticsearch's default limit of 1000 fields. Only specify field mappings for the fields you wish to search on or query. See https://www.elastic.co/guide/en/kibana/master/development-plugin-saved-objects.html#_mappings

id before after diff
url 6 9 +3
Unknown metric groups

API count

id before after diff
share 135 137 +2

History

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

Copy link
Member

@ppisljar ppisljar left a comment

Choose a reason for hiding this comment

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

LGTM

@vadimkibana vadimkibana merged commit c0bf054 into elastic:master Sep 28, 2021
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Sep 28, 2021
@kibanamachine
Copy link
Contributor

💚 Backport successful

Status Branch Result
7.x

This backport PR will be merged automatically after passing CI.

kibanamachine added a commit that referenced this pull request Sep 28, 2021
Co-authored-by: Vadim Kibana <82822460+vadimkibana@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport Deprecated - use backport:version if exact versions are needed Feature:SharingURLs Short URLs and Share URL features release_note:feature Makes this part of the condensed release notes Team:APM All issues that need APM UI Team support v7.16.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement new short URL service Human readable short URLs
9 participants