[APM] Add UI Indices runtime configuration

[ogupte]

Closes #45247
Adds UI Indices runtime configuration

• index configuration in settings page
• defaults to kibana.yml configuration values

[elasticmachine]

💔 Build Failed

• continuous-integration/kibana-ci/pull-request
• Commit: fc6e503864d34818a61bc6735b995f421c20d0a7

[formgeist]

@ogupte Would it be possible to rename all things "UI indices" to simply "Indices". So in the sidenav and page title.

[sorenlouv]

Is the problem that injectDefaultVars has to return synchronously?
Or could you fetch the saved object here?

[ogupte]

I do believe it has to be synchronous, but regardless: the issue here is that the value of apm_oss.transactionIndices could have changed via the UI Indices settings UI since it was injected here during plugin startup. So I'll have to make an API call for it whenever the client requires this index.

[sorenlouv]

Ahh yes ofc. Instead of making a separate endpoint for this, can you call the existing endpoint that exposes the ui indices?

[sorenlouv]

Is it necessary to say it comes from a hook? I got a little confused with that name at first.

Suggested change

	const callApmApiFromHook = useCallApmApi();
	const callApmApi = useCallApmApi();

[dgieselaar]

Naming conflicts with the function argument to useFetcher.

[sorenlouv]

What about using calApmApi from the hook everywhere?

  const callApmApi = useCallApmApi();

  const { data = [], status, refetch } = useFetcher(
   () => callApmApi({ pathname: `/api/apm/settings/ui-indices` }),
    [callApmApi]
  );

I think it's confusing having two different instances of it.

[sorenlouv]

Why refetch on Cancel?

[ogupte]

cancel behavior resets the values to whatever was previously saved, which can be accomplished by calling refetch. Alternatively i could add more to the component state to store the initial values, and set that when cancel is clicked.

[sorenlouv]

Ah, makes sense. Seems simpler to just refetch 👍

[sorenlouv]

Why preservePreviousData: false?

[dgieselaar]

Do these need translations?

[dgieselaar]

Naming conflicts with the function argument to useFetcher.

[dgieselaar]

nit: I think it's generally more robust to initially create an empty object for the form values, and then merge them synchronously inside of the render function. This explicitly prioritizes user changes over network data, and can help eliminate situations where the user will lose data due to a race condition

[dgieselaar]

Translation?

[dgieselaar]

Further down, I thought configuration was an object, but it is a string, so maybe something like configurationName would be helpful.

[dgieselaar]

I'm having a bit of a hard time understanding what this function does. I guess it's getting all the values from apm_oss? Would it be better to just explicitly define that here? Seems like deepValues is not being used either? e.g.:

return Object.values(apmIndices.apm_oss);

[ogupte]

the purpose of flatValues as opposed to Object.values(apmIndices.apm_oss); is to keep getApmIndicesList generic. If we add a new index configuration, e.g. xpack.apm.ui.someNewIndex, then it gets included in getApmIndicesList without having to change that function. I open to changing this, but that was my original motivation.

[dgieselaar]

perhaps an enum would limit the amount of times we have to (re)define this?

[dgieselaar]

Is it possible to use this function for getApmIndices as well? Or are there subtle differences that I'm missing?

[ogupte]

the difference is that getApmIndices returns the resolved static/runtime indices, while this function returns more of a breakdown of the resolution, which informs the UI how to render the settings page. i.e. getApmIndices could be derived from the result of this function, not the other way around.

[dgieselaar]

I've seen this a couple of times now as well, should we use a constant for this?

[dgieselaar]

out of curiosity, should we always intercept server errors and log them explicitly, or does that happen automatically?

[ogupte]

Your right, it's not necessary here. I'll remove these.

[elasticmachine]

💔 Build Failed

• continuous-integration/kibana-ci/pull-request
• Commit: ef644865009e4b387b7f89574c54c04a582a1d13

[elasticmachine]

💔 Build Failed

• continuous-integration/kibana-ci/pull-request
• Commit: 5e191ede30b2b435bba4372dbd91cd3f8635dbd0

[elasticmachine]

💔 Build Failed

• continuous-integration/kibana-ci/pull-request
• Commit: 84bf62b58659b6700fa31904617adde8007353bb

[sorenlouv]

I'm not following this. Why would getApmIndices not return unique values? Or rather: is it important whether they are unique?

[sorenlouv]

Ahh, now I see. This is specifically for addFilterForLegacyData? I found getApmIndicesList a little confusing. Perhaps you can call Object.values(await getApmIndices(server)) from getParamsForSearchRequest and delete getApmIndicesList because I don't see the necessity for removing duplicates.

[sorenlouv]

There is both getApmIndicesList and listUiIndices. That seems a little ambiguous.

[sorenlouv]

This is the nice thing about having identical structures in the config and saved object. It's straightforward to merge them 👍

[sorenlouv]

I forget: did we settle on whether runtime index configuration should be per user, or across all users? And whether anyone should be able to change it or only admins?

[sorenlouv]

Suggested change

	const indexPatternName = await callApmApi({
	const transactionIndices = await callApmApi({

[sorenlouv]

Instead of filtering on the backend, what about simply returning all config options:

return await getApmIndices(server);

It's not a big response, so the savings ar negligible.

[sorenlouv]

This way you can get rid of get_ui_index.ts

[sorenlouv]

Shouldn't this file be located in lib/settings/ui_indices?

[sorenlouv]

Why is the reducer above necessary? Currently only keys in apm_oss is supported anyway. So wouldn't this work?

Suggested change

	return await storeApmUiIndicesSavedObject(server, uiIndicesSavedObject);
	return await storeApmUiIndicesSavedObject(server, uiIndices);

[sorenlouv]

Is this only necessary to satisfy the savedObjectsClient.get interface?

[ogupte]

correct, it satisfies the SavedObjectAttributes type

[sorenlouv]

Currently getApmIndices is called twice for each request. First to get the list of APM indices, and afterwards to verify whether it is an APM index that was queries or not.

What do you think about caching getApmIndices for a short amount of time, eg. 5 seconds? That way if we make several concurrent requests hopefully they won't all have to call getApmIndices. At worst they'll only do it once per request.

[sorenlouv]

As mentioned elsewhere I don't think this method is necessary.

[sorenlouv]

text is mostly used for full-text searches. It will go through the analyzer, will be tokenized and such.
Unless we want that we should use keyword

[sorenlouv]

What about moving this to x-pack/legacy/plugins/apm/server/routes/settings/ui_indices.ts.
Similarly x-pack/legacy/plugins/apm/server/routes/settings.ts should be moved to x-pack/legacy/plugins/apm/server/routes/settings/agent_configuration.ts

[elasticmachine]

💔 Build Failed

• continuous-integration/kibana-ci/pull-request
• Commit: 6262e63ed1d00f0e749d2a24677d432d460cda3f

[elasticmachine]

💔 Build Failed

• continuous-integration/kibana-ci/pull-request
• Commit: d0421ac30ea2a805756442909e6ac34814c8f9c1

[sorenlouv]

Seems like a good move to flatten the option keys 👍

[sorenlouv]

Thanks for doing this everywhere. Tedious work but much more consistent now 👍

[sorenlouv]

Nice to be back on a single line 👍

[sorenlouv]

lgtm!

Couple of TODOs

• decide whether it should be per person or for all
• decide whether specific permissions are required
• possible caching so each request doesn't fetch the saved object twice

I'm okay with merging as-is and then tackle the above questions in follow-up PRs

[sorenlouv]

Each config options can contain a comma separated list of several options. This is mostly used for CCS like apm-*, *:apm-*:

index: [
  'apm-*-transaction-*, *:apm-*-transaction-*',
  'apm-*-span-*, *:apm-*-span-*',
  'apm-*-error-*, *:apm-*-error-*'
]

Can you make sure that queries work with comma-separated strings?

[elasticmachine]

💚 Build Succeeded

• continuous-integration/kibana-ci/pull-request
• Commit: 0409cd2

[sorenlouv]

Great to see this merged. Good job @ogupte ! Will you create the follow-up issues regarding permission model?

[smith]

Test:
Chrome: ✅
Firefox: ✅
IE11: ✅
Safari: ✅