[APM] Moves the APM index creation from server startup #37965
Conversation
💚 Build Succeeded |
|
Pinging @elastic/apm-ui |
|
Pinging @elastic/kibana-app-arch |
There was a problem hiding this comment.
This code looks great, I just had some questions and suggestions. I would approve the PR but when I tested locally I noticed that the problem in the referenced issue still occurs:
So I think either this PR needs more work to fix the issue or we should update the PR description to clarify that it doesn't fix the reported problem.
| return await savedObjectsClient.get('index-pattern', apmIndexPattern.id); | ||
| } catch (error) { | ||
| // if GET fails, then create a new index pattern saved object | ||
| return await savedObjectsClient.create( |
There was a problem hiding this comment.
Does savedObjectsClient return an error with a status code, e.g. 404? If so, then maybe we should check this before trying to create the index pattern. If it's some other kind of status code it seems like we'd probably want to surface it in the UI as an initialization failure or something.
There was a problem hiding this comment.
We attempt to lazily create the index pattern here as a convenience, but if a failure occurs, the APM UI shows the user some messaging that the APM index pattern is required to use certain features. The user is then directed to a tutorial where they can explicitly create the index pattern.
| pathname: `/api/apm/index_pattern` | ||
| }); | ||
| } catch (error) { | ||
| return; |
There was a problem hiding this comment.
Do we want to swallow errors here? It seems like it might be a better UX to show an EuiCallOut with an error message (e.g. "APM failed to initialize" along with the status code and error message), or maybe just a toast notification. This way a user can report this info to support or on the Discuss forums.
There was a problem hiding this comment.
This doesn't happen as part of initialization, but rather attempts to lazily create the APM index pattern when the UI attempts to load it. This memoized function is expected to return undefined if no index pattern is present (in this case, there was a failure to lazily create it). When undefined, the UI shows messaging to the user that the APM index pattern is required to use certain features, and the user is directed to explicitly create it.
| const defaultErrorHandler = (err: Error) => { | ||
| // eslint-disable-next-line | ||
| console.error(err.stack); | ||
| throw Boom.boomify(err, { statusCode: 400 }); |
There was a problem hiding this comment.
Minor nit: is there any benefit to making this more informative by providing different status codes? For example, if the error thrown is due to a JS error (e.g. accessing a property on undefined) then this would be a 500.
There was a problem hiding this comment.
This code looks great, I just had some questions and suggestions. I would approve the PR but when I tested locally I noticed that the problem in the referenced issue still occurs:
So I think either this PR needs more work to fix the issue or we should update the PR description to clarify that it doesn't fix the reported problem.
In my testing, it has been working as expected. The intention is to create the APM index pattern automatically, when the user performs and action which requires it. In my testing, if i start kibana with an empty ES cluster, i see no errors like this until after the APM home page loads (which requires the apm index pattern for the kuery bar to work).
💚 Build Succeeded |
Stand up a fresh ES cluster with no data. Start Kibana and navigate to discover and other plugins. Make sure they don't show any missing data errors. Then navigate to APM, and assert that you don't see any of the message of a missing apm-index (the index pattern will have been automatically created). Navigate to settings and see the |
💔 Build Failed |
💔 Build Failed |
There was a problem hiding this comment.
I followed your testing instructions and everything went as expected.
However, when I clicked on the apm-* index pattern, I got an error that no matching indices were found:
This seemed contrary to your comment that "the index will have been automatically created" so I just wanted to call it out for you.
you are correct! i meant to say "the index pattern will have been automatically created" not the index itself. |
…er startup to savedObject request for the index pattern. It first checks if the index pattern is saved, if not it creates it.
ogupte
force-pushed
the
apm-37499-move-apm-index-creation
branch
from
20dbef2 to
04185f0
Compare
💚 Build Succeeded |
* [APM] Closes elastic#37499 by moving the APM index creation from server startup to savedObject request for the index pattern. It first checks if the index pattern is saved, if not it creates it. * [APM] provide more meaninful status codes in the default error handler
* [APM] Closes elastic#37499 by moving the APM index creation from server startup to savedObject request for the index pattern. It first checks if the index pattern is saved, if not it creates it. * [APM] provide more meaninful status codes in the default error handler
* [APM] Closes #37499 by moving the APM index creation from server startup to savedObject request for the index pattern. It first checks if the index pattern is saved, if not it creates it. * [APM] provide more meaninful status codes in the default error handler
* [APM] Closes #37499 by moving the APM index creation from server startup to savedObject request for the index pattern. It first checks if the index pattern is saved, if not it creates it. * [APM] provide more meaninful status codes in the default error handler
Closes #37499 by moving the APM index creation from server startup to savedObject request for the index pattern. It first checks if the index pattern is saved, if not it creates it.