metricbeat: add integration TestIndexTotalFieldsLimitNotReached #41698

AndersonQ · 2024-11-20T08:31:24Z

Proposed commit message

metricbeat: add integration TestIndexTotalFieldsLimitNotReached

The new integration test TestIndexTotalFieldsLimitNotReached ensures events with at least 500 new dynamically mapped fields can be ingested.

This new test will start failing when the metricbeat mapping has increased to the point it isn't possible to add a new event with 500 dynamically mapped fields. It should act as a safe guard to prevent us from releasing a version where the static mapping is too close to the total fields limit.

Checklist

My code follows the style guidelines of this project
~~[ ] I have commented my code, particularly in hard-to-understand areas~~
~~[ ] I have made corresponding changes to the documentation~~
~~[ ] I have made corresponding change to the default configuration files~~
I have added tests that prove my fix is effective or that my feature works
~~[ ] I have added an entry in CHANGELOG.next.asciidoc or CHANGELOG-developer.next.asciidoc.~~

Disruptive User Impact

N/A

How to test this PR locally

build x-pack/metricbeat
- cd x-pack/metricbeat
- go build .
adjust metricbeat.yml to point to your stack and make sure to include setup.kibana.host
run ./metricbeat setup
generate an event with 500 dynamic fields. Just use the same for loop the test uses
ingest the event:

POST metricbeat-9.0.0/_doc
{ 
   ...
}

it should succeed
change the index.mapping.total_fields.limit for the metricbeat index template:
- go to your-kibna/app/management/data/index_management/templates/metricbeat-9.0.0
- metricbeat-9.0.0 > Manage > Edit > 3 Index settings
- change index.mapping.total_fields.limit to 10000:

"mapping": {
      "total_fields": {
        "limit": "10000"
      }

rollover the index

POST metricbeat-9.0.0/_rollover

try to ingest the same event again, you should get an error like:

{
  "error": {
    "root_cause": [
      {
        "type": "document_parsing_exception",
        "reason": "[1:378] failed to parse: Limit of total fields [10000] has been exceeded while adding new fields [11]"
      }
    ],
    "type": "document_parsing_exception",
    "reason": "[1:378] failed to parse: Limit of total fields [10000] has been exceeded while adding new fields [11]",
    "caused_by": {
      "type": "illegal_argument_exception",
      "reason": "Limit of total fields [10000] has been exceeded while adding new fields [11]"
    },
    "failure_store": "not_enabled"
  },
  "status": 400
}

Related issues

Relates Known issue 8.15.x field limit reached on Metricbeat #41614

mergify · 2024-11-20T08:32:28Z

This pull request does not have a backport label.
If this is a bug or security fix, could you label this PR @AndersonQ? 🙏.
For such, you'll need to label your PR with:

The upcoming major version of the Elastic Stack
The upcoming minor version of the Elastic Stack (if you're not pushing a breaking change)

To fixup this pull request, you need to add the backport labels for the needed
branches, such as:

backport-8./d is the label to automatically backport to the 8./d branch. /d is the digit

mergify · 2024-11-20T08:32:29Z

backport-8.x has been added to help with the transition to the new branch 8.x.
If you don't need it please use backport-skip label and remove the backport-8.x label.

The new integration test TestIndexTotalFieldsLimitNotReached ensures events with at least 500 new dynamically mapped fields can be ingested.

elasticmachine · 2024-11-20T10:43:01Z

Pinging @elastic/elastic-agent-data-plane (Team:Elastic-Agent-Data-Plane)

mauri870 · 2024-11-21T11:37:09Z

x-pack/metricbeat/tests/integration/setup_integration_test.go

+		"-E", "output.file.enabled=false")
+	procState, err := metricbeat.Process.Wait()
+	require.NoError(t, err, "metricbeat setup failed")
+	require.Equal(t, 0, procState.ExitCode(), "metircbeat setup failed: incorrect exit code")


Suggested change

require.Equal(t, 0, procState.ExitCode(), "metircbeat setup failed: incorrect exit code")

require.Equal(t, 0, procState.ExitCode(), "metricbeat setup failed: incorrect exit code")

mauri870 · 2024-11-21T11:42:00Z

x-pack/metricbeat/tests/integration/setup_integration_test.go

+	require.NoError(t, err, "could not send request to send event to ES")
+	defer resp.Body.Close()
+
+	failuremsg := fmt.Sprintf("filed to ingest events with %d new fields. If this test fails it likely means the current `index.mapping.total_fields.limit` for metricbeat index (%s) is close to be reached. Check the logs to see why the envent was not ingested", totalFields, index)


Suggested change

failuremsg := fmt.Sprintf("filed to ingest events with %d new fields. If this test fails it likely means the current `index.mapping.total_fields.limit` for metricbeat index (%s) is close to be reached. Check the logs to see why the envent was not ingested", totalFields, index)

failuremsg := fmt.Sprintf("failed to ingest events with %d new fields. If this test fails it likely means the current `index.mapping.total_fields.limit` for metricbeat index (%s) is close to be reached. Check the logs to see why the event was not ingested", totalFields, index)

mauri870 · 2024-11-21T11:42:53Z

x-pack/metricbeat/tests/integration/setup_integration_test.go

+	defer resp.Body.Close()
+
+	failuremsg := fmt.Sprintf("filed to ingest events with %d new fields. If this test fails it likely means the current `index.mapping.total_fields.limit` for metricbeat index (%s) is close to be reached. Check the logs to see why the envent was not ingested", totalFields, index)
+	if !assert.Equal(t, http.StatusCreated, resp.StatusCode, failuremsg) {


Could use assert.NotEqual here

not really. I want to assert it's equal, if the assertion fails, then the extra code runs to help to explain what went wrong

mauri870 · 2024-11-21T11:45:58Z

x-pack/metricbeat/tests/integration/setup_integration_test.go

+	ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second)
+	defer cancel()
+
+	r, err := http.NewRequestWithContext(ctx, http.MethodPost, endpoint, bytes.NewBuffer(event))


I recall seeing tests that need to interact with Elasticsearch that use the libbeat es client. I think this is preferable to avoid code duplication.

mauri870

LGTM Left some comments, but overall the test seems correct to me.

The linter error seems to be the same one that Lee faced this week, caused by an old version of golangci-lint.

belimawr · 2024-11-22T15:09:27Z

x-pack/metricbeat/tests/integration/setup_integration_test.go

+		_, _, err = integration.HttpDo(t, http.MethodDelete, templateURL)
+		require.NoErrorf(t, err, "cleanup failed: could not remove index template %s", index)
+		_, _, err = integration.HttpDo(t, http.MethodDelete, policyURL)
+		require.NoErrorf(t, err, "cleanup failed: could not ilm policy %s", index)


Typo:

Suggested change

require.NoErrorf(t, err, "cleanup failed: could not ilm policy %s", index)

require.NoErrorf(t, err, "cleanup failed: could not remove ilm policy %s", index)

belimawr · 2024-11-22T15:10:50Z

x-pack/metricbeat/tests/integration/setup_integration_test.go

+	// ensure datastream/index template/ ilm policy is set before running the test
+	cleanUpES()


What do you mean?

You're removing them all, however you wrote about ensuring they're set... I'm confused on what you're trying to accomplish here.

belimawr · 2024-11-22T15:11:32Z

x-pack/metricbeat/tests/integration/setup_integration_test.go

+		"-E", "setup.kibana.port="+kURL.Port(),
+		"-E", "output.elasticsearch.protocol=http",
+		"-E", "output.elasticsearch.hosts=['"+esURL.String()+"']",
+		"-E", "output.file.enabled=false")


Why do you need to set the file output to false?

Suggested change

"-E", "output.file.enabled=false")

belimawr · 2024-11-22T15:15:07Z

x-pack/metricbeat/tests/integration/setup_integration_test.go

+		"-E", "output.file.enabled=false")
+	procState, err := metricbeat.Process.Wait()
+	require.NoError(t, err, "metricbeat setup failed")
+	require.Equal(t, 0, procState.ExitCode(), "metricbeat setup failed: incorrect exit code")


[Suggestion]
Add the exit code to the error message.

Suggested change

require.Equal(t, 0, procState.ExitCode(), "metricbeat setup failed: incorrect exit code")

require.Equalf(t, 0, procState.ExitCode(), "metricbeat setup failed: incorrect exit code: %d", procState.ExitCode())

x-pack/metricbeat/tests/integration/setup_integration_test.go

cmacknz · 2024-11-22T21:35:27Z

Just to confirm, if you revert #41640 this test fails immediately?

cmacknz · 2024-11-22T21:36:58Z

x-pack/metricbeat/tests/integration/setup_integration_test.go

+	require.NoError(t, err, "could not send request to send event to ES")
+	defer resp.Body.Close()
+
+	failuremsg := fmt.Sprintf("failed to ingest events with %d new fields. If this test fails it likely means the current `index.mapping.total_fields.limit` for metricbeat index (%s) is close to be reached. Check the logs to see why the event was not ingested", totalFields, index)


Suggested change

failuremsg := fmt.Sprintf("failed to ingest events with %d new fields. If this test fails it likely means the current `index.mapping.total_fields.limit` for metricbeat index (%s) is close to be reached. Check the logs to see why the event was not ingested", totalFields, index)

failuremsg := fmt.Sprintf("failed to ingest events with %d new fields. If this test fails it likely means the current `index.mapping.total_fields.limit` for metricbeat index (%s) is close to being reached. Check the logs to see why the event was not ingested", totalFields, index)

The new integration test TestIndexTotalFieldsLimitNotReached ensures events with at least 500 new dynamically mapped fields can be ingested. (cherry picked from commit 8a2724a)

cmacknz · 2024-11-22T21:38:14Z

x-pack/metricbeat/tests/integration/setup_integration_test.go

+	"github.com/elastic/beats/v7/libbeat/version"
+)
+
+func TestIndexTotalFieldsLimitNotReached(t *testing.T) {


I would add some explicit documentation for why this test exists and that it is a regression test for #41614 specifically.

This test is likely to fail for seemingly unrelated changes so let's go overboard with documenting the problem it is detecting.

cmacknz · 2024-11-22T21:39:27Z

Just to confirm, if you revert #41640 this test fails immediately?

I see this was answered in the description reading it again 👍

The new integration test TestIndexTotalFieldsLimitNotReached ensures events with at least 500 new dynamically mapped fields can be ingested. (cherry picked from commit 8a2724a)

…tic#41698) The new integration test TestIndexTotalFieldsLimitNotReached ensures events with at least 500 new dynamically mapped fields can be ingested.

…) (#41757) The new integration test TestIndexTotalFieldsLimitNotReached ensures events with at least 500 new dynamically mapped fields can be ingested. (cherry picked from commit 8a2724a) Co-authored-by: Anderson Queiroz <[email protected]>

…) (#41758) The new integration test TestIndexTotalFieldsLimitNotReached ensures events with at least 500 new dynamically mapped fields can be ingested. (cherry picked from commit 8a2724a) Co-authored-by: Anderson Queiroz <[email protected]>

…) (#41759) The new integration test TestIndexTotalFieldsLimitNotReached ensures events with at least 500 new dynamically mapped fields can be ingested. (cherry picked from commit 8a2724a) Co-authored-by: Anderson Queiroz <[email protected]>

…) (#41768) The new integration test TestIndexTotalFieldsLimitNotReached ensures events with at least 500 new dynamically mapped fields can be ingested. (cherry picked from commit 8a2724a) Co-authored-by: Anderson Queiroz <[email protected]>

AndersonQ self-assigned this Nov 20, 2024

botelastic bot added the needs_team Indicates that the issue/PR needs a Team:* label label Nov 20, 2024

AndersonQ force-pushed the 3786-test-for-field-limit-exceed branch from 779abce to b9722e4 Compare November 20, 2024 08:32

mergify bot added the backport-8.x Automated backport to the 8.x branch with mergify label Nov 20, 2024

AndersonQ added the Team:Elastic-Agent-Data-Plane Label for the Agent Data Plane team label Nov 20, 2024

botelastic bot removed the needs_team Indicates that the issue/PR needs a Team:* label label Nov 20, 2024

AndersonQ added backport-8.15 Automated backport to the 8.15 branch with mergify backport-8.16 Automated backport with mergify labels Nov 20, 2024

AndersonQ force-pushed the 3786-test-for-field-limit-exceed branch from b9722e4 to 5fa28a7 Compare November 20, 2024 10:06

AndersonQ changed the title ~~wip: TestIndexTotalFieldsLimit~~ metricbeat: add integration TestIndexTotalFieldsLimitNotReached Nov 20, 2024

metricbeat: add integration TestIndexTotalFieldsLimitNotReached

9e88437

The new integration test TestIndexTotalFieldsLimitNotReached ensures events with at least 500 new dynamically mapped fields can be ingested.

AndersonQ marked this pull request as ready for review November 20, 2024 10:42

AndersonQ requested a review from a team as a code owner November 20, 2024 10:42

AndersonQ requested review from rdner and mauri870 November 20, 2024 10:43

add test cleanup

df48d3b

AndersonQ force-pushed the 3786-test-for-field-limit-exceed branch from 5fa28a7 to df48d3b Compare November 20, 2024 13:12

AndersonQ marked this pull request as draft November 20, 2024 14:13

AndersonQ changed the title ~~metricbeat: add integration TestIndexTotalFieldsLimitNotReached~~ WIP: metricbeat: add integration TestIndexTotalFieldsLimitNotReached Nov 20, 2024

AndersonQ changed the title ~~WIP: metricbeat: add integration TestIndexTotalFieldsLimitNotReached~~ metricbeat: add integration TestIndexTotalFieldsLimitNotReached Nov 20, 2024

AndersonQ marked this pull request as ready for review November 20, 2024 15:53

mauri870 reviewed Nov 21, 2024

View reviewed changes

mauri870 approved these changes Nov 21, 2024

View reviewed changes

fix typos

858b596

belimawr self-requested a review November 22, 2024 15:06

belimawr reviewed Nov 22, 2024

View reviewed changes

pr changes

5836a66

AndersonQ merged commit 8a2724a into elastic:main Nov 22, 2024
18 of 21 checks passed

cmacknz reviewed Nov 22, 2024

View reviewed changes

pierrehilbert added the backport-8.17 Automated backport with mergify label Nov 25, 2024

mergify bot mentioned this pull request Nov 25, 2024

[8.17](backport #41698) metricbeat: add integration TestIndexTotalFieldsLimitNotReached #41768

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

metricbeat: add integration TestIndexTotalFieldsLimitNotReached #41698

metricbeat: add integration TestIndexTotalFieldsLimitNotReached #41698

AndersonQ commented Nov 20, 2024 •

edited

Loading

mergify bot commented Nov 20, 2024

mergify bot commented Nov 20, 2024

elasticmachine commented Nov 20, 2024

mauri870 Nov 21, 2024

mauri870 Nov 21, 2024

mauri870 Nov 21, 2024

AndersonQ Nov 21, 2024

mauri870 Nov 21, 2024

mauri870 left a comment

belimawr Nov 22, 2024

belimawr Nov 22, 2024

belimawr Nov 22, 2024

belimawr Nov 22, 2024

cmacknz commented Nov 22, 2024 •

edited

Loading

cmacknz Nov 22, 2024

cmacknz Nov 22, 2024

cmacknz commented Nov 22, 2024

	require.Equal(t, 0, procState.ExitCode(), "metircbeat setup failed: incorrect exit code")
	require.Equal(t, 0, procState.ExitCode(), "metricbeat setup failed: incorrect exit code")

	failuremsg := fmt.Sprintf("filed to ingest events with %d new fields. If this test fails it likely means the current `index.mapping.total_fields.limit` for metricbeat index (%s) is close to be reached. Check the logs to see why the envent was not ingested", totalFields, index)
	failuremsg := fmt.Sprintf("failed to ingest events with %d new fields. If this test fails it likely means the current `index.mapping.total_fields.limit` for metricbeat index (%s) is close to be reached. Check the logs to see why the event was not ingested", totalFields, index)

	require.NoErrorf(t, err, "cleanup failed: could not ilm policy %s", index)
	require.NoErrorf(t, err, "cleanup failed: could not remove ilm policy %s", index)

		// ensure datastream/index template/ ilm policy is set before running the test
		cleanUpES()

	require.Equal(t, 0, procState.ExitCode(), "metricbeat setup failed: incorrect exit code")
	require.Equalf(t, 0, procState.ExitCode(), "metricbeat setup failed: incorrect exit code: %d", procState.ExitCode())

metricbeat: add integration TestIndexTotalFieldsLimitNotReached #41698

metricbeat: add integration TestIndexTotalFieldsLimitNotReached #41698

Conversation

AndersonQ commented Nov 20, 2024 • edited Loading

Proposed commit message

Checklist

Disruptive User Impact

How to test this PR locally

Related issues

mergify bot commented Nov 20, 2024

mergify bot commented Nov 20, 2024

elasticmachine commented Nov 20, 2024

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

mauri870 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

cmacknz commented Nov 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmacknz commented Nov 22, 2024

AndersonQ commented Nov 20, 2024 •

edited

Loading

cmacknz commented Nov 22, 2024 •

edited

Loading