Skip to content

Short URLs#107859

Merged
vadimkibana merged 18 commits intoelastic:masterfrom
vadimkibana:short-urls-2
Sep 28, 2021
Merged

Short URLs#107859
vadimkibana merged 18 commits intoelastic:masterfrom
vadimkibana:short-urls-2

Conversation

@vadimkibana
Copy link
Copy Markdown
Contributor

@vadimkibana vadimkibana commented Aug 8, 2021 •
edited
Loading

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 a review from a team September 7, 2021 12:11
@vadimkibana vadimkibana requested review from a team as code owners September 7, 2021 12:11
peteharverson
peteharverson approved these changes Sep 7, 2021
View reviewed changes
Copy link
Copy Markdown
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 - DEPRECATED Use Team:obs-ux-infra_services. label Sep 7, 2021
@elasticmachine
Copy link
Copy Markdown
Contributor

elasticmachine commented Sep 7, 2021

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

alisonelizabeth
alisonelizabeth approved these changes Sep 7, 2021
View reviewed changes
Copy link
Copy Markdown
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.

kertal
kertal approved these changes Sep 7, 2021
View reviewed changes
Copy link
Copy Markdown
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.

smith
smith approved these changes Sep 7, 2021
View reviewed changes
Copy link
Copy Markdown
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.

stratoula
stratoula approved these changes Sep 8, 2021
View reviewed changes
Copy link
Copy Markdown
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 :)

ppisljar
ppisljar reviewed Sep 16, 2021
View reviewed changes
src/plugins/share/common/url_service/url_service.ts
* Common URL Service client interface for server-side and client-side.
*/
export class UrlService {
export class UrlService<D = unknown> {
Copy link
Copy Markdown
Contributor

@ppisljar ppisljar Sep 16, 2021

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
Copy Markdown
Contributor Author

@vadimkibana vadimkibana Sep 20, 2021

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?

ppisljar
ppisljar reviewed Sep 16, 2021
View reviewed changes
@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 - DEPRECATED Use Team:obs-ux-infra_services. labels Sep 20, 2021
@elasticmachine
Copy link
Copy Markdown
Contributor

elasticmachine commented Sep 20, 2021

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 - DEPRECATED Use Team:obs-ux-infra_services. label Sep 20, 2021
@vadimkibana
Copy link
Copy Markdown
Contributor Author

vadimkibana commented Sep 22, 2021

@elasticmachine merge upstream

@vadimkibana
Copy link
Copy Markdown
Contributor Author

vadimkibana commented Sep 23, 2021

@elasticmachine merge upstream

@vadimkibana
Copy link
Copy Markdown
Contributor Author

vadimkibana commented Sep 26, 2021

@elasticmachine merge upstream

@vadimkibana
Copy link
Copy Markdown
Contributor Author

vadimkibana commented Sep 27, 2021

@elasticmachine merge upstream

@kibanamachine
Copy link
Copy Markdown
Contributor

kibanamachine commented Sep 27, 2021

💛 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

ppisljar
ppisljar approved these changes Sep 27, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@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
@vadimkibana @kibanamachine
Short URLs (elastic#107859)
15edc7b
@kibanamachine kibanamachine mentioned this pull request Sep 28, 2021
Merged
@kibanamachine
Copy link
Copy Markdown
Contributor

kibanamachine commented Sep 28, 2021

💚 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
@kibanamachine @vadimkibana
Short URLs (#107859) (#113228)
8e6b834 
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

Reviewers

@smith smith smith approved these changes

@kertal kertal kertal approved these changes

@alisonelizabeth alisonelizabeth alisonelizabeth approved these changes

@peteharverson peteharverson peteharverson approved these changes

@ppisljar ppisljar ppisljar approved these changes

@stratoula stratoula stratoula approved these changes

Assignees

No one assigned

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 - DEPRECATED Use Team:obs-ux-infra_services. v7.16.0 v8.0.0

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Implement new short URL service Human readable short URLs

9 participants

@vadimkibana @elasticmachine @kibanamachine @smith @kertal @alisonelizabeth @peteharverson @ppisljar @stratoula