[Behavioral Analytics] Analytics collections use DSL instead of ILM for data retention management #100033

kderusso · 2023-09-28T18:42:42Z

Behavioral analytics has traditionally used ILM to manage data retention. Starting with 8.11.0, this will change. Analytics collections created prior to 8.11.0 will continue to use their existing ILM policies, but new analytics collections will be managed using DSL.

How this changes the datastream:

Create an analytics collection using the following command:

PUT /_application/analytics/all-about-puggles

Next, get the backing data stream. For the above example, the command would be:

GET _data_stream/behavioral_analytics-events-all-about-puggles

Versions prior to 8.11 would return the following information:

{
  "data_streams": [
    {
      "name": "behavioral_analytics-events-all-about-puggles",
      "timestamp_field": {
        "name": "@timestamp"
      },
      "indices": ...
      "generation": 1,
      "_meta": {
        "description": "Built-in template applied by default to behavioral analytics event data streams.",
        "managed": true
      },
      "status": "GREEN",
      "template": "behavioral_analytics-events-default",
      "ilm_policy": "behavioral_analytics-events-default_policy",
      "hidden": false,
      "system": false,
      "allow_custom_routing": true,
      "replicated": false
    }
  ]
}

Note that the ilm_policy is set to behavioral_analytics-events-default_policy. You can see details about this policy with the call GET _ilm/policy/behavioral_analytics-events-default_policy.

Now, the following information is returned:

{
  "data_streams": [
    {
      "name": "behavioral_analytics-events-all-about-puggles",
      "timestamp_field": {
        "name": "@timestamp"
      },
      "indices": ...
      "generation": 1,
      "_meta": {
        "description": "Built-in template applied by default to behavioral analytics event data streams.",
        "managed": true
      },
      "status": "GREEN",
      "template": "behavioral_analytics-events-default",
      "lifecycle": {
        "enabled": true,
        "data_retention": "180d"
      },
      "hidden": false,
      "system": false,
      "allow_custom_routing": true,
      "replicated": false
    }
  ]
}

Note the lifecycle block with enabled set to true and data_retention set to 180d. Additionally note that ilm_policy is no longer specified.

Note: This PR did not change the default data retention period, only the system with which data retention is managed.

elasticsearchmachine · 2023-09-29T17:08:51Z

Pinging @elastic/ent-search-eng (Team:Enterprise Search)

elasticsearchmachine · 2023-09-29T17:08:51Z

Hi @kderusso, I've created a changelog YAML for you.

andreidan

Thanks for looking into Data stream lifecycle Kathleen.

Exciting you're looking to use DSL as a default ❤️

I left a few minor suggestions and questions.

docs/changelog/100033.yaml

...e-resources/src/main/resources/entsearch/analytics/behavioral_analytics-events-settings.json

...full-cluster-restart/javaRestTest/java/org/elasticsearch/entsearch/FullClusterRestartIT.java

carlosdelest

Overall LGTM. Should we add some BWC tests to check this keeps working on older versions?

...full-cluster-restart/javaRestTest/java/org/elasticsearch/entsearch/FullClusterRestartIT.java

afoucret · 2023-10-02T12:19:32Z

...full-cluster-restart/javaRestTest/java/org/elasticsearch/entsearch/FullClusterRestartIT.java

+ * in compliance with, at your election, the Elastic License 2.0 or the Server
+ * Side Public License, v 1.
+ */
+package org.elasticsearch.entsearch;


It seems to me that we should use org.elasticsearch.xpack.application instead of org.elasticsearch.entsearch.
Also I would prefer to organize those tests by application (analytics here).

Suggested change

package org.elasticsearch.entsearch;

package org.elasticsearch.xpack.application.analytics;

I kept it as entsearch assuming that we'll have additional cases in the module outside of BA in the future that we want to test a full restart with.

There is two different points here.

First org.elasticsearch.entsearch has been abandoned since a while and your package name should at least be org.elasticsearch.xpack.application.

Second, I would like to keep the code (including tests) of each application separated since it ease refactoring. If you want to add a test for another application (let's say for search application), I would recommend that is goes into it own class which would be in the right package.

Thanks for the explanation. I have refactored the package name, but as discussed elsewhere I'd like to keep the pattern now of using FullClusterRestartIT. We can refactor this at a later date if it becomes burdensome.

kderusso · 2023-10-02T12:48:45Z

@carlosdelest

Overall LGTM. Should we add some BWC tests to check this keeps working on older versions?

The FullClusterRestartIT should handle backward compatibility testing, what else did you have in mind?

afoucret

Overall it looks good but I think we can benefits from some minor changes in tests.

Also, I will look more carefully at the behavior described in @andreidan comment later this today.

afoucret · 2023-10-02T12:19:48Z

...full-cluster-restart/javaRestTest/java/org/elasticsearch/entsearch/FullClusterRestartIT.java

+import java.io.IOException;
+import java.util.List;
+
+public class FullClusterRestartIT extends ParameterizedFullClusterRestartTestCase {


I would prefer a more explicit test naming:

Suggested change

public class FullClusterRestartIT extends ParameterizedFullClusterRestartTestCase {

public class DataStreamLifecycleMigrationIT extends ParameterizedFullClusterRestartTestCase {

I disagree, this is a very lightweight test right now and there's precedent for FullClusterRestartIT in other modules. I think it lowers the barrier of entry to adding more tests to keep it as-is until we deem otherwise.

I agree with @afoucret as this currently holds tests for analytics. But I think @kderusso is aiming for a pattern that is present in other areas of the codebase - doing a specific test for cluster restarting, where we can add new tests for migrations.

I'd stick with a common codebase pattern even if my first gut feeling is to create a more specific, per-feature level test class.

Both patterns exist into the codebase (eg org.elasticsearch.xpack.restart.WatcherMappingUpdateIT.
I prefer using one class per test with an explicit naming.

BTW, each tests has it own requirements before being executed (cluster settings, ...) and having separate class per concern makes it easier to manage this intentionally without accidentally modifying other tests.

I'm being worn down. 🙂 If you both feel strongly that I change it I will, otherwise I'll keep it as is.

...full-cluster-restart/javaRestTest/java/org/elasticsearch/entsearch/FullClusterRestartIT.java

carlosdelest · 2023-10-02T14:00:35Z

The FullClusterRestartIT should handle backward compatibility testing, what else did you have in mind?

I'm not familiar with that kind of integration testing. Does it handle a mixed cluster setup, in which different nodes have different versions? Would that be needed for this use case?

kderusso · 2023-10-05T18:23:40Z

I'm not familiar with that kind of integration testing. Does it handle a mixed cluster setup, in which different nodes have different versions? Would that be needed for this use case?

@carlosdelest this handles migrating/updating to a new version. We do have BWC tests on the API classes, but I'm not sure we have anything pluggable for template registry. I'll take a look though!

carlosdelest · 2023-10-06T07:17:15Z

@carlosdelest this handles migrating/updating to a new version. We do have BWC tests on the API classes, but I'm not sure we have anything pluggable for template registry. I'll take a look though!

I'm taking a closer look and the restart test seems to do the same thing that a BwC mixed test should do. LGTM, thanks for checking!

carlosdelest

LGTM

andreidan

LGTM, thanks for working on this Kathleen

kderusso · 2023-10-09T17:46:49Z

@afoucret I cannot merge this until you re-review, can you please review this? Thank you.

afoucret · 2023-10-10T08:21:59Z

x-pack/plugin/ent-search/qa/full-cluster-restart/build.gradle

+assert Version.fromString(VersionProperties.getVersions().get("elasticsearch")).getMajor() == 8:
+  "If we are targeting a branch other than 8, we should enable migration tests"
+
+BuildParams.bwcVersions.withWireCompatible(v -> v.after("8.8.0") && v.before("8.12.0")) { bwcVersion, baseName ->


I understand v.after("8.8.0") since the entsearch module was not existing before but I think v.before("8.12.0") should be part of the test class. Indeed we need to make sure we will be able to test future migrations (eg. between 8.12.0 and 8.13.0)

The best way to do this is to use assumeTrue as the first line of your test:

public void testBehavioralAnalyticsDataRetention() throws Exception { assumeTrue('No need to run', getOldClusterVersion().before(V_8_12_0)) // ... Your test code }

assumeTrue will cause the test to be skipped if the condition version is not met (there is also a assumeFalse version)

Thanks for the feedback - changed this, but would appreciate a second set of eyes.

I will check in the CI runs if everything behave as expected.

Thank you for the double check.

This is ok for me

Unblocking the PR to be merged when you considered it ready

...ster-restart/javaRestTest/java/org/elasticsearch/xpack.application/FullClusterRestartIT.java

kderusso added 2 commits September 28, 2023 10:33

Update Analytics to use DLM instead of ILM

eb53339

Update IT tests

7c0c400

elasticsearchmachine added the v8.11.0 label Sep 28, 2023

kderusso added 5 commits September 28, 2023 14:55

Cleanup comment

7bb71a3

Fiddling with tests

128e9d3

Fixed upgraded/new DLM verification

229cbc6

Cleanup dup files

cf0818b

Update ILM legacy check in integration test

8c054dc

kderusso changed the title ~~[Behavioral Analytics] Analytics collections created with DLM instead of ILM~~ [Behavioral Analytics] Analytics collections use DLM instead of ILM for data retention management Sep 29, 2023

kderusso marked this pull request as ready for review September 29, 2023 17:07

elasticsearchmachine added the needs:triage Requires assignment of a team area label label Sep 29, 2023

kderusso added >feature :EnterpriseSearch/Application Enterprise Search Team:Enterprise Search Meta label for Enterprise Search team and removed needs:triage Requires assignment of a team area label labels Sep 29, 2023

Update docs/changelog/100033.yaml

b352038

Update changelog

8555ad3

kderusso requested a review from a team September 29, 2023 17:11

andreidan reviewed Oct 2, 2023

View reviewed changes

carlosdelest approved these changes Oct 2, 2023

View reviewed changes

...full-cluster-restart/javaRestTest/java/org/elasticsearch/entsearch/FullClusterRestartIT.java Outdated Show resolved Hide resolved

afoucret reviewed Oct 2, 2023

View reviewed changes

Minor PR feedback

6fdf3e3

afoucret previously requested changes Oct 2, 2023

View reviewed changes

Fix typo in changelog

cd66871

kderusso changed the title ~~[Behavioral Analytics] Analytics collections use DLM instead of ILM for data retention management~~ [Behavioral Analytics] Analytics collections use DSL instead of ILM for data retention management Oct 2, 2023

mattc58 removed the v8.11.0 label Oct 4, 2023

mattc58 added the v8.12.0 label Oct 4, 2023

kderusso added 3 commits October 5, 2023 14:13

Update references from 8.11.0 to 8.12.0

aea3978

Merge branch 'main' into kderusso/behavioral_analytics_dlm

194c390

Ensure existing ILM backed indices continue to be managed

6e02421

kderusso added the test-update-serverless label Oct 5, 2023

kderusso requested review from afoucret, andreidan, carlosdelest and a team October 5, 2023 19:47

carlosdelest approved these changes Oct 6, 2023

View reviewed changes

andreidan approved these changes Oct 6, 2023

View reviewed changes

afoucret reviewed Oct 10, 2023

View reviewed changes

kderusso added 3 commits October 11, 2023 11:20

Merge branch 'main' into kderusso/behavioral_analytics_dlm

05361d9

PR feedback - package rename

ebf9c0d

Update test version checks

aa9667b

afoucret reviewed Oct 12, 2023

View reviewed changes

...ster-restart/javaRestTest/java/org/elasticsearch/xpack.application/FullClusterRestartIT.java Outdated Show resolved Hide resolved

kderusso added 3 commits October 12, 2023 08:09

Fix intellij refactoring fail

81f6ae5

Fix package dir name

584d3c1

Merge branch 'main' into kderusso/behavioral_analytics_dlm

e7a4cad

afoucret approved these changes Oct 12, 2023

View reviewed changes

kderusso merged commit 1dbe928 into elastic:main Oct 13, 2023
13 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Behavioral Analytics] Analytics collections use DSL instead of ILM for data retention management #100033

[Behavioral Analytics] Analytics collections use DSL instead of ILM for data retention management #100033

kderusso commented Sep 28, 2023 •

edited

elasticsearchmachine commented Sep 29, 2023

elasticsearchmachine commented Sep 29, 2023

andreidan left a comment

carlosdelest left a comment

afoucret Oct 2, 2023

kderusso Oct 2, 2023

afoucret Oct 10, 2023

kderusso Oct 11, 2023

kderusso commented Oct 2, 2023

afoucret left a comment

afoucret Oct 2, 2023

kderusso Oct 5, 2023

carlosdelest Oct 6, 2023

afoucret Oct 10, 2023 •

edited

kderusso Oct 11, 2023

carlosdelest commented Oct 2, 2023

kderusso commented Oct 5, 2023

carlosdelest commented Oct 6, 2023

carlosdelest left a comment

andreidan left a comment

kderusso commented Oct 9, 2023

afoucret Oct 10, 2023

afoucret Oct 10, 2023

kderusso Oct 11, 2023

afoucret Oct 12, 2023

kderusso Oct 12, 2023

afoucret Oct 12, 2023

	package org.elasticsearch.entsearch;
	package org.elasticsearch.xpack.application.analytics;

	public class FullClusterRestartIT extends ParameterizedFullClusterRestartTestCase {
	public class DataStreamLifecycleMigrationIT extends ParameterizedFullClusterRestartTestCase {

[Behavioral Analytics] Analytics collections use DSL instead of ILM for data retention management #100033

[Behavioral Analytics] Analytics collections use DSL instead of ILM for data retention management #100033

Conversation

kderusso commented Sep 28, 2023 • edited

elasticsearchmachine commented Sep 29, 2023

elasticsearchmachine commented Sep 29, 2023

andreidan left a comment

Choose a reason for hiding this comment

carlosdelest left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kderusso commented Oct 2, 2023

afoucret left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

afoucret Oct 10, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlosdelest commented Oct 2, 2023

kderusso commented Oct 5, 2023

carlosdelest commented Oct 6, 2023

carlosdelest left a comment

Choose a reason for hiding this comment

andreidan left a comment

Choose a reason for hiding this comment

kderusso commented Oct 9, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kderusso commented Sep 28, 2023 •

edited

afoucret Oct 10, 2023 •

edited