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

[EPM] Conditionally generate ES index pattern name based on dataset_is_prefix #89870

Merged
merged 10 commits into from
Feb 8, 2021

Conversation

skh
Copy link
Contributor

@skh skh commented Feb 1, 2021

Summary

Implements #88307

This now uses the following index patterns in the generated index templates. A manifest.yml file for a data stream status looking like this:

title: Apache status metrics
release: experimental
type: metrics
[...]

results in metrics-apache.status-*, and this:

title: Apache status metrics
release: experimental
type: metrics
dataset_is_prefix: true
[...]

results in metrics-apache.status.*-*

How to test this

Have a local registry running so that you can edit a data stream manifest.yml for testing.

Pick a package that contains a data stream without dataset_is_prefix set -- that would be any package we have in the registry now, I'm using the Apache package and the status data stream. Install it, and verify that the template for the data stream has been generated correctly:

  • in Kibana dev tools, look at the template list with GET _cat/templates and fetch a specific template for a data stream, e.g. with GET _index_template/metrics-apache.status
  • verify that the index_patterns property for this template looks like this: ["metrics-apache.status-*"] and that the priority of the template is 200.

Now uninstall the package and change the data stream manifest.yml to add the dataset_is_prefix: true setting. (You might need to restart the local registry after that change.)

  • in Kibana dev tools, fetch the template again
  • verify that the index_patterns property now looks like this: ["metrics-apache.status.*-*"] and that the priority of the template is 150.

Checklist

Delete any items that are not applicable to this PR.

@skh skh added Feature:EPM Fleet team's Elastic Package Manager (aka Integrations) project release_note:skip Skip the PR/issue when compiling release notes Team:Fleet Team label for Observability Data Collection Fleet team v7.12.0 v8.0.0 labels Feb 1, 2021
@jalvz
Copy link
Contributor

jalvz commented Feb 2, 2021

APM fix for the issue described here: elastic/apm-server#4669

@spalger
Copy link
Contributor

spalger commented Feb 3, 2021

jenkins test this

@skh
Copy link
Contributor Author

skh commented Feb 4, 2021

@elasticmachine merge upstream

@skh skh marked this pull request as ready for review February 4, 2021 16:13
@elasticmachine
Copy link
Contributor

Pinging @elastic/fleet (Feature:EPM)

@skh skh self-assigned this Feb 4, 2021
@skh skh requested review from ruflin, a team, nchaulet, jen-huang and jfsiii February 4, 2021 16:17
@skh
Copy link
Contributor Author

skh commented Feb 4, 2021

Apologies for requesting so many reviews manually, I couldn't request one from @elastic/fleet.

@@ -23,27 +23,29 @@ expect.addSnapshotSerializer({
});

test('get template', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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)
  ...

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 for the pointer, I've added a unit test for installTemplate().

@skh skh force-pushed the 88307-use-dataset-is-prefix branch from e678cfc to 8b58f46 Compare February 5, 2021 11:13
@skh skh force-pushed the 88307-use-dataset-is-prefix branch from 31a83f4 to 4e543cb Compare February 8, 2021 10:20
@skh
Copy link
Contributor Author

skh commented Feb 8, 2021

@jen-huang Thanks for your review, this is ready for another look!

@skh
Copy link
Contributor Author

skh commented Feb 8, 2021

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

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

@jen-huang jen-huang self-requested a review February 8, 2021 15:27
Copy link
Contributor

@jen-huang jen-huang left a comment

Choose a reason for hiding this comment

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

Thanks for adding those install tests. One small question but not a blocker, otherwise LGTM 👍🏻

});

// This test is not an API integration test with Kibana
// We want to test here if the template is valid and for this we need a running ES instance.
// 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({
Copy link
Contributor

Choose a reason for hiding this comment

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

why are the two ES requests here changed to the generic transport.request?

Copy link
Contributor Author

@skh skh Feb 8, 2021

Choose a reason for hiding this comment

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

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.)

Copy link
Contributor

Choose a reason for hiding this comment

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

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

@skh skh merged commit c306a44 into elastic:master Feb 8, 2021
skh added a commit to skh/kibana that referenced this pull request Feb 8, 2021
…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 <[email protected]>
skh added a commit that referenced this pull request Feb 9, 2021
…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 <[email protected]>

Co-authored-by: Kibana Machine <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:EPM Fleet team's Elastic Package Manager (aka Integrations) project release_note:skip Skip the PR/issue when compiling release notes Team:Fleet Team label for Observability Data Collection Fleet team v7.12.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants