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

Generic UI search API #61657

Closed
4 tasks done
joshdover opened this issue Mar 27, 2020 · 22 comments
Closed
4 tasks done

Generic UI search API #61657

joshdover opened this issue Mar 27, 2020 · 22 comments
Assignees
Labels
enhancement New value added to drive a business result NeededFor:Core UI REASSIGN from Team:Core UI Deprecated label for old Core UI team Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc

Comments

@joshdover
Copy link
Contributor

joshdover commented Mar 27, 2020

To power Global Search (#57576) and future features like ChatOps, we should provide a general framework for searching for various objects, apps, and other locations in the UI that a user may want to find quickly.

This framework should allow plugins to provide different "data sources" of which may be backed by Elasticsearch indices or other items (eg. application links).

The search framework should be able to take a search inputs, call each data source, and aggregate results into something sensible.

Tasks

@joshdover joshdover added Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc enhancement New value added to drive a business result REASSIGN from Team:Core UI Deprecated label for old Core UI team labels Mar 27, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-platform (Team:Platform)

@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-core-ui (Team:Core UI)

@pgayvallet
Copy link
Contributor

pgayvallet commented Apr 16, 2020

Regarding global architecture (actual required savedObjects dataSource(s) implementation will have it's own discussion)

dataSource registration and architecture

This framework should allow plugins to provide different "data sources" of which may be backed by Elasticsearch indices or other items (eg. application links).

In an ideal world, datasources would only be registered on the server-side, and the aggregation logic would also be contained on the server. core would then expose both a server-side contract API to be able to use this 'global search' from the server part of plugins, and an endpoint (with an associated client) to be able to use it from the public part of plugins.

However, we already know that some searchable objects (applications) are currently only available from the client-side (see #57576 (comment))

The first consequence is that we will need to allow datasources to be registered from the client-side.

A corollary from this first statement is that searching from the server-side will not provide all results, as it would not be possible to 'query' the client-side for client-side-registered sources.

We got an architectural decision to make here:

Option A: only allow to register dataSources from the client-side

Data providers would only be able to register dataSources from the client-side. If the datasource requires to perform calls to the server-side to retrieve their results, it will be considered an implementation detail.

Pros:

  • reduce API exposure
  • more consistent, every datasources are registered the same way
  • results aggregation logic only performed one, on the client-side

Cons:

  • more work for dataSource implementations, as it would requires to create the endpoint on the server and call it from the dataSource
  • Even if chatOps has been excluded from the requirements from now, it will make it way harder to reintegrate it later

Option B: allow to register dataSources from both client-side and server-side

As most of the currently identified datasources (AFAIK, all of them except the applications one, see #58049) needs to perform server calls anyway, it would be possible to allow plugins to register dataSources from either the client-side or the server-side.

Pros:

  • less work for dataSource providers, can just register it from the server-side
  • would be able to have a server-side GS API with 'degraded' search results (without the client-side datasources' results)
  • (follow-up of previous point) probably more 'future needs' proof, as we would be able to expose a server-side API for GS (i.e for chatOps), even if degraded.

Cons:

  • Implementation more complex for core. We'll have to handle results aggregation twice: once on the server to fetch all server-side datasources, then on the client-side to merge the server results and the client-side dataSources results

I'm leaning to option B, but it's not a strong opinion, and would really like to have the other's vision on that.

POC interface for the dataSource providers

This is a base, naive contract to open the discussion, please comment on that.

// are fields missing here?
interface GlobalSearchResult {
  // the main text/title to display in search results UI
  title: string;
  // the type of result. also displayed as line2 in search result UI
  // I.E `dashboard`, `application`
  type: string;
  // EUI icon name to display for the search result.
  // Optional, will display a default (or no icon?) if not provided
  icon?: string;
  // The url to navigate to this result
  url: string;
  // optional, priority/ordering of the result
  order?: number;
}

interface GlobalSearchDataSourceResponse {
  success: boolean;
  // only if success == true
  results?: GlobalSearchResult[];
  // only if success == false
  error?: string;
}

// type instead of just a `(term: string, options: {}): Promise<GlobalSearchDataSourceResponse>` function
// to allow potential addition to the `dataSource` contract in the future.  
type GlobalSearchDataSource = {
  search(term: string, options: {}): Promise<GlobalSearchDataSourceResponse>;
};

// would be used like
coreSetup.globalSearch.addDataSource(myDataSource);

Remarks / Questions:

  • How to handle redirecting to a result's associated page.

In current POC implem, I just added a GlobalSearchResult.url string that is supposed to be the url to redirect to. But it's definitly not enough:

In stage 1 of GS, we will only have objects contained in the kibana instance we are searching for, however in next stage we will have to display result from other clusters' kibana instances, and therefor be able to redirect to an url outside of current kibana's domain.

Navigating to an internal object should probably be performed using application.navigateTo, meaning we would need to have a { application: string, path?: string } object in the search result to be able to navigate to it.
Navigating to an object outside of the kibana app will need a 'real' absolute url to the object, meaning we will need an url: string for that.

Would something like

interface GlobalSearchResult {
  // ...
  navigateTo: { absUrl: string } | { application: string; path?: string };
}

suit the need?

  • Results scoring / ordering

In current POC, I just added a GlobalSearchResult.order? field to allow dataSources to return an order/priority for their results. It's probably not good enough.

As we are going to fetch results from multiple dataSources, I'm not really sure how to properly introduce a global/centralised scoring mechanism, as it would relies on the underlying dataSources to score their results, not to the GS service/aggregator, which mean distinct dataSources would have different scoring mechanism, therefor making them biased and useless.

I don't see any good option on that problem, so any insight is welcome.

  • Do you see anything else?

Last point, should I create a RFC for that issue? If so, should I do it right now or after we find answers to these problematics?

ping @elastic/kibana-core-ui

@ryankeairns
Copy link
Contributor

Thanks @pgayvallet , I'll bring this up at our sync today.

@kobelb your input would be appreciated here as well!

@myasonik
Copy link
Contributor

dataSource registration and architecture

Don't really have an opinion here but do have a hazy recollection from an early meeting with @stacey-gammon that a client-side only implementation ended up being the leading idea. Stacey, do you remember this any better than I do? Have any thoughts on @pgayvallet's proposal? 👆

POC interface for the dataSource providers

How to handle redirecting to a result's associated page?

navigateTo: { absUrl: string } | { application: string; path?: string }; seems good 👍 Haven't had a look into it but would this, in the future, let us switch spaces as well or would we need more?

Results scoring / ordering

Having a "global" level of scoring would be cool but I'm not sure that it's necessary. I think it'd be totally reasonable if the results were grouped by their type so all that the aggregator would then need to do is order the types themselves. If we could do some sort of scoring on which type is most likely to be the one we want, that could be neat but I think also just having a predefined hierarchy would be fine.

Anything else?

  1. Not sure how strict you're being with types, but would GlobalSearchResult.type be better as an enum/string literal union of known types?
  2. How future looking do we want the GlobalSearchResult interface to be? Other things that we've at one point or another mentioned possible being displayed that would need to exist here could be: space; for certain types, author, last edited, created at.

@pgayvallet
Copy link
Contributor

@myasonik Thanks for the input

Don't really have an opinion here but do have a hazy recollection from an early meeting with @stacey-gammon that a client-side only implementation ended up being the leading idea

TBH the hybrid server/client implementation is mostly driven by the need to able to use GS for external APIs such as ChatOps. I really don't know how much using GS outside of the UI is a real / realistic need and if the degraded search results would be acceptable. If it's not, my option A is probably the way to go. Not really sure who should have the last word on that...

Haven't had a look into it but would this, in the future, let us switch spaces as well or would we need more?

application.navigateTo stays on the same space. Allowing to display results from other spaces and then navigate to them would need an additional information.
The struct could probably be enhanced to add a space optional property then.

I see two issues here however:

  • Not sure how to programmatically change space. @kobelb is there a programmatic API for that? Can we also navigate to like a specific url of a given space.
  • (probably more important) ATM the GS service is supposed to live in core, meaning that we won't be able to access the space plugin API anyway, which is kinda blocker.

If we choose option A regarding dataSources (only allow to register them from the client-side), we no longer need the GlobalSearchResult type to be serializable, as instances will only live on the client side, and we could then let the datasource providers handle the navigation themselves.

navigateTo: { absUrl: string } | { application: string; path?: string } would become navigateTo: () => void

with a example implementation of navigateTo: () => application.navigateTo('dashboard', { path: 'my-dashboard'})

This would allow dataSources providers to have more control on how they should handle access to the result, while definitely breaking the possibility to have a server-side API counterpart. This is a high-impact decision.

Having a "global" level of scoring would be cool but I'm not sure that it's necessary. I think it'd be totally reasonable if the results were grouped by their type so all that the aggregator would then need to do is order the types themselves.

This would be very doable. The main question I see here is: is ordering by type sufficient? I'm thinking mostly of results from other spaces or even other kibana instances. Should dashboard type results from another cluster have the same priority than dashboard results from current kibana instance?

Maybe we should order by dataSource instead?

type GlobalSearchDataSource = {
  getDisplayPriority(): number;
  search(term: string, options: {}): Promise<GlobalSearchDataSourceResponse>;
};

wdyt?

Not sure how strict you're being with types, but would GlobalSearchResult.type be better as an enum/string literal union of known types?

I'm mixed on this one. The major downside I see with that is that 3rd party plugins would not be able to register dataSources fortypes not included in this static list defined in the kibana codebase, which seems kinda dealbreaker in term of extensibility?

How future looking do we want the GlobalSearchResult interface to be?

Ideally, any field added at a later time would be optional, to allow BWC with the existing dataSources. This is why we need to look a little further than stage 1 here.

@joshdover
Copy link
Contributor Author

The first consequence is that we will need to allow datasources to be registered from the client-side.

A corollary from this first statement is that searching from the server-side will not provide all results, as it would not be possible to 'query' the client-side for client-side-registered sources.

@pgayvallet and I discussed this over Slack and we decided it's probably best to go with the hybrid approach, though preferring datasources to be registered on the server side when possible:

  • Makes supporting ChatOps in the future easier since we won't have to move most data sources to the server.
  • Allows us to avoid solving some of the hard problems right now: specifically how to return results for application links on the server-side. There are a few options to solving this, but we don't need to for GS and we can defer that work to whenever ChatOps becomes a priority.

Navigating to an internal object should probably be performed using application.navigateTo, meaning we would need to have a { application: string, path?: string } object in the search result to be able to navigate to it.
Navigating to an object outside of the kibana app will need a 'real' absolute url to the object, meaning we will need an url: string for that.

I think we may be able to get away with just using absolute URLs, while still supporting navigating to other spaces and doing in-app SPA navigations. This idea is contingent on being able to parse out the application id from the URL, but I think this algorithm would be quite simple:

  • If all 3 of these criteria are true:
    • The domain of the URL (if present) matches the current domain
    • The pathname of the URL starts with the current basePath (eg. /mybasepath/s/my-space)
    • The pathname segment after the basePath matches an application route (eg. /app/<id>/)
    • Then: match the pathname segment to the corresponding application and do the SPA navigation to that application using the remaining pathname segment
  • Otherwise, do a full page navigation (window.location.assign())

Why this should work:

  • When navigating within the same space, the basePath will match since it already contains the space prefix. Core won't need to have any knowledge of how spaces works.
  • When navigating to another space, the basePath will not match and we will do a full-page navigation. We need to do a full page nav anyways in order to reload the UiSettings and the user's capabilities for the new space. (at least with how spaces works currently).
  • When navigating to another Kibana cluster, will obviously need to a do a full page refresh.

Is there any case I'm missing here?

If we choose option A regarding dataSources (only allow to register them from the client-side), we no longer need the GlobalSearchResult type to be serializable, as instances will only live on the client side, and we could then let the datasource providers handle the navigation themselves.

I think this creates a non-trivial amount of work later when we do want to support chatops. I think having some constraints on what results can do is a useful design decision for future requirements.

@stacey-gammon
Copy link
Contributor

Thanks for tagging me @myasonik. Here are my thoughts:

dataSource registration and architecture
Option A: only allow to register dataSources from the client-side
Option B: allow to register dataSources from both client-side and server-side

I'm leaning towards option B to avoid BWC issues down the road and pave the way for chat ops.

Urls & storage

UrlGenerators are registered client side, and the plan for now is to keep them that way, but to have the links accessible on server side via a simple helper:

// On server side
createGotoLink(urlGenerator) =>
  `${getDomain}/${getBasePath}/goto/${urlGenerator.id}?state=${risonEncode(JSON.stringify(urlGenerator.state))}`

You wouldn't need that though until actually generating the links to give to an end user. The following code could exist on server or client:

DashboardPlugin{
  setup(core) {
    core.globalSearch.addFinderDestination({
        search(term: string, options: {}): Promise<GlobalSearchDataSourceResponse> => {
          const [coreStart, ] = await core.getStartServices();
          const dashboards = coreStart.savedObjectClient.find('dashboard', term);
          return dashboards.map(dashboard => {
             title: dashboard.title,
             score: dashboard.score  // Can you reuse the score returned from ES here instead of "order"?
             urlGenerator: {
               id: DASHBOARD_URL_GENERATOR,
               {
                 id: dashboard.id,
               }
             }
          })
        }
    });
  }
}

I need to think through this a little more, but I want to be sure we are thinking through backward compatibility here.

With the URL Generator service, what is backward compatible is the id + state. NOT the url returned from it. I just gave a presentation on persistable state registries. I need to think more about where URLS fit into this picture.

Like, what if someone created a plugin that supported pinning search hits from this service? The pinned hit would be stored in a saved object with a url string with no way to support migrations from the owner of that url. If you stored the URL generator state, there is a migration path.

Thinking through a couple options:

  1. Instead of returning the url string, return the url generator id and the url generator state. Create the href links on the fly, but you will still have the data so if someone did want to create that feature above, you'd have the data there that would be safe to store in a saved object in a BWC way (there is a way for an author of a URL generator to migrate the state they support). Chat ops could do this conversion too. Downside is that users of our API don't have a real URL to expose, they'd need to query yet another service to get it.

  2. Encourage every implementor to preserve backward compatibility of the URL they use. They could use the goto links we are thinking of creating for the Url Generators:

http://domain/goto/{id}?state={risonEncodedUrlGeneratorState}

^^ that doesn't exist yet but it should to support bwc server side url links, because the app will grab the correct generator, decode the state, and use the Url generator which will have any registered migrations.

  1. Have an official stance that these links should not be persisted, they are only good for the current version of Kibana and are meant to be used for "on the fly" searches, not storage. Would make the "pin this search result" feature more difficult.

Order

Regarding order. Maybe instead of order there should be a concept of "match score"? Just a thought.

Terminology

It may be too late in the game to change the terminology of "Global Search", though I do think this is going to be confusing. We have search services inside the data plugin. Maybe like "Global finder services" or "global resource locator" (... or, universal resource locator? 😄 )

Even if you are sold on Global Search, I really think you should steer away from "data source" because it's already used by canvas "data sources" and used heavily inside the ingestion manager.

Some suggested alternatives:

  • Resource
  • Page
  • FinderPage
  • Destination? FinderDestination?
  • FinderItem
  • LandingLocation?
  • I wouldn't use SearchSource either because we already use that name for saved searches and "search" is already confusing.

The one given is that every item must have a url destination, right?

@myasonik
Copy link
Contributor

myasonik commented Apr 16, 2020

Navigation

I like @stacey-gammon's first two options much more than using absolute URL strings. Relying on a bunch of string manipulation to pull logic out of it just tends to spider with condionals over time so having the actual backing data feels much safer.

No preference between the first two options but I think taking the stance that we will not support persisting URLs will come back to bite us so I'd avoid thinking of URLs as single use actions.

Ordering results

[Ordering by type] would be very doable. The main question I see here is: is ordering by type sufficient? I'm thinking mostly of results from other spaces or even other kibana instances. Should dashboard type results from another cluster have the same priority than dashboard results from current kibana instance?

I've got no idea. Almost seems like a product question? But also one that we might need data on from users... Also seems like an answer that is likely to change over time and/or how big the customer is. How much flexibility could we bake into our ordering strategies?

Maybe we should order by dataSource instead?

I'm not sure I follow on how this is different... What falls into a single if not a single type?

Maybe instead of order there should be a concept of "match score"?

Wouldn't that not be possible across multiple ?

GlobalSearchResult.type type (string or enum)

The major downside I see with [an enum] is that 3rd party plugins would not be able to register dataSources fortypes not included in this static list defined in the kibana codebase, which seems kinda dealbreaker in term of extensibility?

So, as a string, how would sorting results work if we have a/a bunch of type(s) that we don't know?

Something that would need a bit more plumbing but we could allow plugins to register types which could then be mixed into the types we provide to have both a known set of types and what to do with them while also letting developers extend Kibana.

@stacey-gammon
Copy link
Contributor

Something that would need a bit more plumbing but we could allow plugins to register types which could then be mixed into the types we provide to have both a known set of types and what to do with them while also letting developers extend Kibana.

We have this kind of thing in use already in quite a few places, but I'm actually thinking we should discourage its use. Check out #62815 for more details. Although I'll reiterate here because that issue doesn't really touch on the type mappings.

You can see how URL generators does this here: https://github.com/elastic/kibana/blob/drilldowns/examples/url_generators_examples/public/plugin.tsx#L31

If you can type check this though, you know the id. E.g. you are doing something like

// If you use type mappings, this will be automatically typed and ensure
// that the state is of the shape the `FOO` generator requires
 urlGenerator.get(FOO).createUrl({ fooBar: 'hi; })

Because otherwise you are doing something like this:, which you can't type check anyway:

// We can't type check this, just have to hope where-ever this persisted data came from is
// using the right state.
 urlGenerator.get(id).createUrl(state)

So, in the former case, what I talk about in that issue, is that we should discourage direct access with the plugin if you know, at runtime, the type you want, and instead, get it off the registrators plugin directly. e.g.

 // Correctly typed
  foo.getUrlGenerator().createUrl(state);

Then you don't need typescript mappings at all.

@kobelb
Copy link
Contributor

kobelb commented Apr 17, 2020

The hybrid approach is likely what we'll end up with long-term. As long as it doesn't drastically expand the scope and throw off timelines, implementing it from the start seems reasonable. Cloud UI 2.0 will be making various customizations to Kibana, including augmenting the global search bar with Cloud specific datasources. At the moment, the plan is to have all integration with Cloud APIs be performed client-side, so either approach will work in this regard.

Spaces essentially hijack the base path to include the space url prefix. Internal to the spaces plugin, there's a utility function to augment a path with the space url prefix. Unfortunately, there isn't any functionality currently that creates a URL to navigate to an application in a different space. However, as long as we know the space, this is definitely possible. For the initial implementation, we'll likely want to only search within the existing space. The saved-object API only works for a single space at the moment, and can't be used to search across spaces. Long-term, this is definitely something that we'll want to support though.

The url generators service is currently in the share plugin which appears to be focused on supporting the share context menus. If we start utilizing it here, it feels like it should be moved elsewhere. One of the first datasources which we'll want to search is saved-objects. We don't want to have to do a SavedObjectsClient::find per saved-object type, so there will likely be a single saved-objects datasource, as opposed to a separate dashboards datasource and a separate visualizations datasource, etc... Assuming we use the url generators, we'll need to find a way to translate from the saved-object type to the url generator ID. We'll also likely need to make changes to allow these URLs to work across spaces, and the spaceID doesn't feel like it's something that should be part of the UrlGeneratorState.

With regard to ranking the search results, switching from an order to a score seems like a good idea. If a datasource is able to use the _score from Elasticsearch, this is the easiest. Otherwise, we'll have to figure out how to emulate the score. There's a section in Elasticsearch: The Definitive Guide which I need to reread before I can really speak to this. Perhaps we should also consider adding a "weight" to data-sources, so that we can boost the results from the most relevant datasources and perhaps long-term allow this to be customized?

@kobelb
Copy link
Contributor

kobelb commented Apr 17, 2020

On further reading, we potentially should not be trying to use the _score of Elasticsearch in this regard. Per https://www.elastic.co/guide/en/elasticsearch/guide/master/practical-scoring-function.html#query-norm

The query normalization factor (queryNorm) is an attempt to normalize a query so that the results from one query may be compared with the results of another.

Even though the intent of the query norm is to make results from different queries comparable, it doesn’t work very well. The only purpose of the relevance _score is to sort the results of the current query in the correct order. You should not try to compare the relevance scores from different queries.

There's also quite a lot that goes into scoring these documents that emulating the score is potentially quite challenging.

For the initial implementation, if we can figure out an amenable design which displays the results grouped by the datasource, it'll let us avoid having to figure this out...

@ryankeairns
Copy link
Contributor

ryankeairns commented Apr 17, 2020

I won't pretend to have entirely followed ALL of that 😄, but as far as grouping results according to some prefixed order goes - I think we could make that UX work.

As we've envisioned this feature, two common use cases typically come to mind:

  1. Help me navigate to an app
  2. Help me find my dashboard/visualization/etc.

If we accept these as our core use cases, then forcing an order of results that returns applications, then saved objects, then anything else (we can hash out what other categories are left) seems like it would still provide the desired outcome we're after.

That said, the ability to define a type in your search would allow users to bring anything to the top. Suppose you could search something like Dashboard: My favorite dash which would, in effect, make saved objects the top (only) result type. If we don't want to get into too much complexity handling user typed strings, perhaps they could even scope their search via a dropdown - in this scenario you'd change 'All' to 'Saved objects', as an example - then input your string in the search input.

I don't know if that helps with scoping down the complexity, but there is flexibility in the UX that I am more than happy to explore.

@bmcconaghy @alexfrancoeur do you have any thoughts?

@joshdover
Copy link
Contributor Author

Assuming we use the url generators, we'll need to find a way to translate from the saved-object type to the url generator ID.

SavedObject types can already register a function to generate a URL to link to that object. We could easily leverage this for the saved objects data source.

This definitely has some overlap with the URL Generator service, and it may make sense to consolidate these mechanisms or provide a bridge of some sort. I'm just trying to think about how we could get a working MVP done with less effort.

Like, what if someone created a plugin that supported pinning search hits from this service? The pinned hit would be stored in a saved object with a url string with no way to support migrations from the owner of that url. If you stored the URL generator state, there is a migration path.

Depending on how this implemented, there isn't necessarily a problem with using absolute URLs. I would expect that a pinned search result is a full result or backed by some object, rather than being a raw URL. So a pinned result may have the form of { dataSource: string; resultId: string; }. When a user performs a search, the data source would lookup the backing result, and we could generate the absolute URL at query time, not at the point in time the result was pinned. That way we don't need to think about BWC since the URL is generated on the current version.

That said, using URL generators may be useful when linking to other clusters where the version of the destination cluster may differ from the cluster the user searched from. But once again, depending on how those data sources are implemented, we could actually call the getInAppUrl function on the destination cluster before returning the results to the searching cluster.

This pattern only works for saved objects though. I'm not sure what else we may want to search for, but if there are other things that are not saved objects that we need to return, using the URL generators may be necessary. But I don't think we necessarily have to use that pattern from the start. I would be more comfortable leveraging URL generators once that pattern has had more time to mature and there are generators implemented for all the things we need.

@pgayvallet
Copy link
Contributor

So, It seems like the biggest questions is regarding the actual url format / mechanism.

@stacey-gammon biggest issues/questions I have with using the urlGenerator service is that

  • This service is in a plugin atm, not in core, and the GS service is supported to be, meaning that with current architecture, we atm will not be able to use it directly from the GS service on the client side to resolve the actual url of a result to redirect to it.
  • it make it way harder for (external) GS API consumers to actually use the url from the results as it forces them to call an additional (actually non-existing) API to resolve the URL from the urlGenerator suggested property.
  • it forces any kind of results to have an associated urlGenerator registered. Which is not necessarily the case for all SO types or for things like applications.

We could choose to go for absolute urls and use the createGotoLink (once available) on the server side:

DashboardPlugin{
  setup(core) {
    core.globalSearch.addFinderDestination({
        search(term: string, options: {}): Promise<GlobalSearchDataSourceResponse> => {
          const [coreStart, ] = await core.getStartServices();
          const dashboards = coreStart.savedObjectClient.find('dashboard', term);
          return dashboards.map(dashboard => {
             title: dashboard.title,
             score: dashboard.score  // Can you reuse the score returned from ES here instead of "order"?
             url: createGotoLink(DASHBOARD_URL_GENERATOR, { id: dashboard.id })
          })
        }
    });
  }
}

However with the implementation you provided

createGotoLink(urlGenerator) =>
  `${getDomain}/${getBasePath}/goto/${urlGenerator.id}?state=${risonEncode(JSON.stringify(urlGenerator.state))}

This kinda go against @joshdover suggestion to parse the url to be able to use navigateTo instead of triggering a full page refresh, as the application and path are not parseable from the generic url createGotoLink returns.

Or would application.navigateTo('goto', { path: urlGenerator.id}) work in that case?

Also, not sure how that would work with space change ?

@joshdover

I think we may be able to get away with just using absolute URLs, while still supporting navigating to other spaces and doing in-app SPA navigations. This idea is contingent on being able to parse out the application id from the URL, but I think this algorithm would be quite simple:
If all 3 of these criteria are true:

  • The domain of the URL (if present) matches the current domain
  • The pathname of the URL starts with the current basePath (eg. /mybasepath/s/my-space)
  • The pathname segment after the basePath matches an application route (eg. /app//)
  • Then: match the pathname segment to the corresponding application and do the SPA navigation to that application using the remaining pathname segment

TBH I'm kinda afraid about

The domain of the URL (if present) matches the current domain

Because of reverse-proxy and middleware that can be present in complex production environment, I think we cannot be certain that the domain in the a server-generated result's url (using server.name ? server.host?) is going to be the domain from the browser's location.

wdyt?

Also

The pathname segment after the basePath matches an application route (eg. /app//)

Because we can define a custom route for an application

appRoute?: string;

We would need to check every registered app to see if it matches. But this is likely not a real issue.

@stacey-gammon
Copy link
Contributor

@stacey-gammon biggest issues/questions I have with using the urlGenerator service is that

Great points @pgayvallet. Seems like returning the url string and using createGotoLink is the way to go, esp with @joshdover's suggestion of returning a resultId that can be used to re-create the link at a later time to avoid persisting absolute urls.

I imagine the createGotoLink could optionally take a "absolute" or "relative" option which I think would then avoid the full page refresh? If we have to use navigateTo then we could return enough information to do something like you suggest as well. I'm not quite sure what the options are for this fn but you'd need more info than what you posted, bc you need the state stored in a query parameter.

const link = createGotoLink(id, state);
const query = url.parse(link).query;
application.navigateTo('goto', { path: urlGenerator.id, query })
 // or
const path = createGotoLink(id, state, { relative: true });
application.navigateTo('goto', { path })

For spaces, don't we want it to look up the object inside the current space only? So the space prefix should be automatically applied based on whatever space the user is in when looking for results?

@pgayvallet
Copy link
Contributor

I'm not quite sure what the options are for this fn but you'd need more info than what you posted, bc you need the state stored in a query parameter.

As long as createGotoLink returns an absolute or relative url, I'd say needed parameters are an implementation detail of the dataSource registrant that core doesn't really need to know about. My only concern with this approach is the client-side parseability of such urls, to be able to perform a application.navigateTo call.

However, for v1 needs and the SO types we need to be searching for, I think I really like @joshdover idea of using the already existing getInAppUrl. The SO type registry being already exposed from core, it would be very easy to retrieve a specific object's associated relative path. Of course, other providers could leverage the urlGenerator service, as the way the results provider are generating their url is an implementation detail.

Last point I'm thinking of, if that if the results exposed to the public GS API should all have absolute urls, we could still allow dataSources to return either absolute or relative urls, and have core results aggregator consolidate relative urls to absolute one. That would remove some logic from the dataSources that are only returning results from the current kibana instance.

I.E

class SomePlugin {
  setup(core) {
    core.globalSearch.addDataSource({
      search(term: string, options: {}): Promise<GlobalSearchDataSourceResponse> {
        return [
          {
            title: 'someObject',
            // relative - will be consolidated to https://current-kibana-instance/base-path/my-app/path-to/some-object
            url: '/my-app/path-to/some-object', 
          },
          {
            title: 'objectFromAnotherCluster',
            // abs - will be kept as it is
            url: 'https://kibana2/path-to/objectFromAnotherCluster', 
          },
        ];
      },
    });
  }
}

wdyt?

For spaces, don't we want it to look up the object inside the current space only?

For v1 we will only be returning results from the current space. For next versions we do want to be able to search for objects outside of current space, and therefor need to be able to generate absolute urls containing the space.

@joshdover
Copy link
Contributor Author

Because of reverse-proxy and middleware that can be present in complex production environment, I think we cannot be certain that the domain in the a server-generated result's url (using server.name ? server.host?) is going to be the domain from the browser's location.

wdyt?

It may be time that we introduce a server.publishAddress config (or similar) for this reason. It will also be required to support chatops since we need to post full complete URLs back to the chat app.

When it's not set, the server can just return absolute paths without the protocol, domain, and port. For example /app/dashboard?id=1234, which the frontend logic would assume is the same domain as the current one. This is similar to your example in the subsequent comment.

@pgayvallet
Copy link
Contributor

It may be time that we introduce a server.publishAddress config (or similar) for this reason

I think it makes sense.

When it's not set, the server can just return absolute paths without the protocol, domain, and port.

One other option would be to construct this publishAddress when unset, using the http config values.

Would be something like (naive implem, there are some trailing slash issues when no basePath, but it's to get the idea):

const publishAddress = `${getServerInfo().protocol}://${httpConfig.host}:${httpConfig.port}/${httpConfig.basePath}`

I feel like this would answer the need in maybe 90% of the deployments while allowing ops to still configure the publishAddress manually if needed.

@stacey-gammon
Copy link
Contributor

Watching the CAH recording and I see that App Search allows the user to tweak the priority of different types of results, so for example, an engineer could give extra weight to github results. Might be worth asking someone on that team how they are solving this problem of ordering results from mixed sources.

@pgayvallet
Copy link
Contributor

pgayvallet commented Apr 24, 2020

As this is a complex feature, I created an RFC with a proposal for the initial version of the globalSearch API: #64284. Let's continue the discussion on the review of this RFC.

@pgayvallet
Copy link
Contributor

Closing as all sub tasks have been addressed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New value added to drive a business result NeededFor:Core UI REASSIGN from Team:Core UI Deprecated label for old Core UI team Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc
Projects
None yet
Development

No branches or pull requests

7 participants