[EPM] Conditionally generate ES index pattern name based on dataset_is_prefix#89870
[EPM] Conditionally generate ES index pattern name based on dataset_is_prefix#89870skh merged 10 commits intoelastic:masterfrom
Conversation
|
APM fix for the issue described here: elastic/apm-server#4669 |
9778948 to
ef78ab6
Compare
ef78ab6 to
1138547
Compare
|
jenkins test this |
|
@elasticmachine merge upstream |
|
Pinging @elastic/fleet (Feature:EPM) |
|
Apologies for requesting so many reviews manually, I couldn't request one from @elastic/fleet. |
There was a problem hiding this comment.
it would be great to have a unit test for installTemplate since that's the method that generates the index pattern and priority based on data stream information. I don't see any tests here that actually test on the dataset_is_prefix property
There was a problem hiding this comment.
Good point. Is there an established way how to mock calls to elasticsearch in unit tests?
I've added unit tests for generateTemplateIndexPattern() and getTemplatePriority() as a start in 8b58f46
There was a problem hiding this comment.
it looks like Kibana core does export some ES mocks, here's an example: https://github.com/elastic/kibana/blob/master/x-pack/plugins/fleet/server/services/epm/packages/_install_package.test.ts#L38
I see that installTemplate passes callCluster to other methods, so I guess you might have to mock ES return values in order of whenever callCluster is called further down the chain. the mocked ES client returns a jest.fn() so mocking the responses could be done like this:
const mockCallCluster = elasticsearchServiceMock.createLegacyScopedClusterClient().callAsCurrentUser
.mockReturnValueOnce({ success: true })
.mockReturnValueOnce({ success: true })
.mockReturnValueOnce(// change mocked response whenever the response matters)
...
There was a problem hiding this comment.
Thanks for the pointer, I've added a unit test for installTemplate().
e678cfc to
8b58f46
Compare
x-pack/plugins/fleet/server/services/epm/elasticsearch/template/template.test.ts
Outdated
Show resolved
Hide resolved
31a83f4 to
4e543cb
Compare
|
@jen-huang Thanks for your review, this is ready for another look! |
|
@elasticmachine merge upstream |
💚 Build SucceededMetrics [docs]
History
To update your PR or re-run it, just comment with: |
jen-huang
left a comment
There was a problem hiding this comment.
Thanks for adding those install tests. One small question but not a blocker, otherwise LGTM 👍🏻
| // If the ES instance takes the template, we assume it is a valid template. | ||
| const { body: response1 } = await es.indices.putTemplate({ | ||
| name: templateName, | ||
| const { body: response1 } = await es.transport.request({ |
There was a problem hiding this comment.
why are the two ES requests here changed to the generic transport.request?
There was a problem hiding this comment.
We're working with index templates v2. The endpoint for these is _index_template instead of _template, and the new ES client provides putIndexTemplate() and getIndexTemplate() methods to interface with that. The integration tests use the legacy client, which doesn't have these methods, so I fall back to the generic method.
At some point someone(TM) should probably tackle getting the new ES client into the integration tests, I didn't because it didn't look obvious at first glance.
For index templates v2 see elastic/elasticsearch#53101
(These tests were disabled for this reason, this PR reenables them.)
There was a problem hiding this comment.
got it, thanks for that context. #74111 is the issue for migrating to new ES client, I just added a note about looking at usages in tests too
…s_prefix (elastic#89870) * Explicitly generate ES index pattern name. * Adjust tests. * Adjust and reenable tests. * Set template priority based on dataset_is_prefix * Refactor indexPatternName -> templateIndexPattern * Add unit tests. * Use more realistic index pattern in test. * Fix unit test. * Add unit test for installTemplate(). Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
…s_prefix (#89870) (#90656) * Explicitly generate ES index pattern name. * Adjust tests. * Adjust and reenable tests. * Set template priority based on dataset_is_prefix * Refactor indexPatternName -> templateIndexPattern * Add unit tests. * Use more realistic index pattern in test. * Fix unit test. * Add unit test for installTemplate(). Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
Summary
Implements #88307
This now uses the following index patterns in the generated index templates. A
manifest.ymlfile for a data streamstatuslooking like this:results in
metrics-apache.status-*, and this:results in
metrics-apache.status.*-*How to test this
Have a local registry running so that you can edit a data stream
manifest.ymlfor testing.Pick a package that contains a data stream without
dataset_is_prefixset -- that would be any package we have in the registry now, I'm using the Apache package and thestatusdata stream. Install it, and verify that the template for the data stream has been generated correctly:GET _cat/templatesand fetch a specific template for a data stream, e.g. withGET _index_template/metrics-apache.statusindex_patternsproperty for this template looks like this:["metrics-apache.status-*"]and that the priority of the template is200.Now uninstall the package and change the data stream
manifest.ymlto add thedataset_is_prefix: truesetting. (You might need to restart the local registry after that change.)index_patternsproperty now looks like this:["metrics-apache.status.*-*"]and that the priority of the template is150.Checklist
Delete any items that are not applicable to this PR.