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

[App Search] Empty Crawler Single Domain view #107694

Merged

Conversation

byronhulcher
Copy link
Contributor

@byronhulcher byronhulcher commented Aug 4, 2021

Summary

Sets up a view and related routes for the Crawler Single Domain view. We retrieve the data for the domain on load and display it in an EuiCode.

Screenshots

Screen Shot 2021-08-04 at 3 24 10 PM

Checklist

Delete any items that are not applicable to this PR.

@byronhulcher byronhulcher added Feature:Plugins release_note:skip Skip the PR/issue when compiling release notes auto-backport Deprecated - use backport:version if exact versions are needed v7.15.0 labels Aug 4, 2021
@byronhulcher byronhulcher requested review from orhantoy and a team August 4, 2021 19:28
pageHeader={{ pageTitle: displayDomainUrl }}
isLoading={dataLoading}
>
<EuiCode>{JSON.stringify(domain, null, 2)}</EuiCode>
Copy link
Contributor

Choose a reason for hiding this comment

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

I like the incremental approach with this PR. ++

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks Rich, we've done it on a few other views, I've found it helpful to show that something's happening on the BE even if we don't have anything fancy to show

Copy link
Contributor

Choose a reason for hiding this comment

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

+++

Copy link
Contributor

@orhantoy orhantoy left a comment

Choose a reason for hiding this comment

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

Overall looks good. Posted a few questions and suggestions but nothing critical.

const { fetchDomainData } = useActions(CrawlerSingleDomainLogic);

const displayDomainUrl = domain
? domain.url
Copy link
Contributor

Choose a reason for hiding this comment

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

Aren't we displaying the domain name only in the standalone UI? Is that something you wanna follow up with or do in this PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

domain.url is domain.name in the API which I believe is only the domain + protocol, and does not include any entry points

pageHeader={{ pageTitle: displayDomainUrl }}
isLoading={dataLoading}
>
<EuiCode>{JSON.stringify(domain, null, 2)}</EuiCode>
Copy link
Contributor

Choose a reason for hiding this comment

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

+++


try {
const response = await http.get(
`/api/app_search/engines/${engineName}/crawler/domains/${domainId}`
Copy link
Contributor

Choose a reason for hiding this comment

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

In this context, this should be OK but I would suspect that with analytics for instance we would want/need to escape the dynamic parts of the URL path.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you elaborate on this? I'm not sure I understand why we would want/need to escape the dynamic parts in the context of analytics.

@@ -66,6 +66,21 @@ export function registerCrawlerRoutes({
})
);

router.get(
{
path: '/api/app_search/engines/{name}/crawler/domains/{id}',
Copy link
Contributor

Choose a reason for hiding this comment

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

The name and id params here read a bit weird to me because they could appear to be related but one if the engine name and the other is the domain ID. I see this is consistent with the other routes but perhaps something to look into and clear up later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah this follows existing but not great patterns, in the future I would like us to be more specific with these names.

Co-authored-by: Orhan Toy <toyorhan@gmail.com>
@byronhulcher byronhulcher enabled auto-merge (squash) August 5, 2021 15:01
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
enterpriseSearch 1460 1462 +2

Async chunks

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

id before after diff
enterpriseSearch 2.1MB 2.1MB +1.8KB

History

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

@byronhulcher byronhulcher merged commit a51787d into elastic:master Aug 5, 2021
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Aug 5, 2021
* New route to retreive data for a single domain

* New CrawlerSingleDomainLogic logic

* New CrawlerSingleDomain view component

* Add CrawlerSingleDomain to CrawlerRouter

* Use different default text for page title while loading

* Apply suggestions from code review

Co-authored-by: Orhan Toy <toyorhan@gmail.com>

Co-authored-by: Orhan Toy <toyorhan@gmail.com>
@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 Aug 5, 2021
* New route to retreive data for a single domain

* New CrawlerSingleDomainLogic logic

* New CrawlerSingleDomain view component

* Add CrawlerSingleDomain to CrawlerRouter

* Use different default text for page title while loading

* Apply suggestions from code review

Co-authored-by: Orhan Toy <toyorhan@gmail.com>

Co-authored-by: Orhan Toy <toyorhan@gmail.com>

Co-authored-by: Byron Hulcher <byronhulcher@gmail.com>
Co-authored-by: Orhan Toy <toyorhan@gmail.com>
streamich pushed a commit to vadimkibana/kibana that referenced this pull request Aug 8, 2021
* New route to retreive data for a single domain

* New CrawlerSingleDomainLogic logic

* New CrawlerSingleDomain view component

* Add CrawlerSingleDomain to CrawlerRouter

* Use different default text for page title while loading

* Apply suggestions from code review

Co-authored-by: Orhan Toy <toyorhan@gmail.com>

Co-authored-by: Orhan Toy <toyorhan@gmail.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:Plugins release_note:skip Skip the PR/issue when compiling release notes v7.15.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants