Clean up / document metrics monitor fields #39413

faec · 2024-05-06T13:06:23Z

Document the semantics of many metrics monitoring variables, and rename some metrics APIs to more clearly indicate their function. As a side effect, fix several metrics reporting / publishing bugs in the Elasticsearch output, including #39146.

Add the output.events.dead_letter metric to distinguish events that were ingested to the dead letter index after a fatal error (previously these events were just reported as "acked").

This could have been a shorter fix, but it was hard to properly test since the metrics were changed from two separate functions with a lot of special cases. I ended up reorganizing the Elasticsearch Publish helpers to make the logic more clear. The new layout makes it much easier to test the error handling and metrics reporting.

The bugs fixed by this refactor are:

When a batch was split, the events in it were not reported to the observer via RetryableErrors. This caused activeEvents to increase permanently even after the events were handled.
When a previously-failed event was ingested to the dead letter index as a raw string, it was reported to the observer as Acked (success). The new logic creates a new dead_letter metric specifically for this case.
When a previously-failed event encountered a fatal error ingesting to the dead letter index:
- It was reported to the observer as both a permanent error and a retryable error, giving incorrect event counts. It's now reported as just a permanent error.
- It was added to the retry list anyway, which would block all ingestion if the error really was fatal since the queue could never advance past that event. It's now dropped, the same as with a permanent error in the main index.
If the Elasticsearch bulk index response was invalid, the associated events were dropped and reported as acknowledged

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

Related issues

Fixes Elasticsearch output can report an incorrect count for active events #39146.

mergify · 2024-05-06T13:07:02Z

This pull request does not have a backport label.
If this is a bug or security fix, could you label this PR @faec? 🙏.
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-v8./d.0 is the label to automatically backport to the 8./d branch. /d is the digit

elasticmachine · 2024-05-06T15:09:08Z

💔 Tests Failed

the below badges are clickable and redirect to their specific view in the CI or DOCS

Expand to view the summary

Build stats

Start Time: 2024-05-06T13:06:41.317+0000
Duration: 122 min 9 sec

Test stats 🧪

Test	Results
Failed	2
Passed	29365
Skipped	2044
Total	31411

Test errors

Expand to view the tests failures

`Build&Test / libbeat-unitTest / TestCollectPublishFailsNone – github.com/elastic/beats/v7/libbeat/outputs/elasticsearch`

Expand to view the error details

 Failed

Expand to view the stacktrace

 === RUN   TestCollectPublishFailsNone
--- FAIL: TestCollectPublishFailsNone (0.00s)
panic: interface conversion: interface {} is nil, not *elasticsearch.encodedEvent [recovered]
	panic: interface conversion: interface {} is nil, not *elasticsearch.encodedEvent

goroutine 122 [running]:
testing.tRunner.func1.2({0x147ce40, 0xc000383bc0})
	/var/lib/jenkins/workspace/PR-39413-1-fd020dae-7cd0-4ea4-9adb-7c7ff81cb105/.gvm/versions/go1.21.9.linux.amd64/src/testing/testing.go:1545 +0x238
testing.tRunner.func1()
	/var/lib/jenkins/workspace/PR-39413-1-fd020dae-7cd0-4ea4-9adb-7c7ff81cb105/.gvm/versions/go1.21.9.linux.amd64/src/testing/testing.go:1548 +0x397
panic({0x147ce40?, 0xc000383bc0?})
	/var/lib/jenkins/workspace/PR-39413-1-fd020dae-7cd0-4ea4-9adb-7c7ff81cb105/.gvm/versions/go1.21.9.linux.amd64/src/runtime/panic.go:914 +0x21f
github.com/elastic/beats/v7/libbeat/outputs/elasticsearch.(*Client).bulkCollectPublishFails(0xc0003eb040, {0xc000029800, 0xafd, 0xc00}, {0xc0001f8000, 0x64, 0x64})
	/var/lib/jenkins/workspace/PR-39413-1-fd020dae-7cd0-4ea4-9adb-7c7ff81cb105/src/github.com/elastic/beats/libbeat/outputs/elasticsearch/client.go:398 +0xdcd
github.com/elastic/beats/v7/libbeat/outputs/elasticsearch.TestCollectPublishFailsNone(0xc0003bc360?)
	/var/lib/jenkins/workspace/PR-39413-1-fd020dae-7cd0-4ea4-9adb-7c7ff81cb105/src/github.com/elastic/beats/libbeat/outputs/elasticsearch/client_test.go:266 +0x256
testing.tRunner(0xc0003eab60, 0x1699f30)
	/var/lib/jenkins/workspace/PR-39413-1-fd020dae-7cd0-4ea4-9adb-7c7ff81cb105/.gvm/versions/go1.21.9.linux.amd64/src/testing/testing.go:1595 +0xff
created by testing.(*T).Run in goroutine 1
	/var/lib/jenkins/workspace/PR-39413-1-fd020dae-7cd0-4ea4-9adb-7c7ff81cb105/.gvm/versions/go1.21.9.linux.amd64/src/testing/testing.go:1648 +0x3ad

`Build&Test / libbeat-goIntegTest / TestClientBulkPublishEventsWithDeadletterIndex – github.com/elastic/beats/v7/libbeat/outputs/elasticsearch`

Expand to view the error details

 Failed

Expand to view the stacktrace

 === RUN   TestClientBulkPublishEventsWithDeadletterIndex
    client_integration_test.go:243: Expecting mapping conflict
--- FAIL: TestClientBulkPublishEventsWithDeadletterIndex (0.39s)

Steps errors

Expand to view the steps failures

`filebeat-goIntegTest - mage goIntegTest`

Took 18 min 15 sec . View more details here
Description: mage goIntegTest

`libbeat-unitTest - mage build unitTest`

Took 12 min 39 sec . View more details here
Description: mage build unitTest

`libbeat-unitTest - mage build unitTest`

Took 9 min 17 sec . View more details here
Description: mage build unitTest

`libbeat-unitTest - mage build unitTest`

Took 9 min 18 sec . View more details here
Description: mage build unitTest

`libbeat-goIntegTest - mage goIntegTest`

Took 15 min 40 sec . View more details here
Description: mage goIntegTest

`libbeat-goIntegTest - mage goIntegTest`

Took 11 min 1 sec . View more details here
Description: mage goIntegTest

`libbeat-goIntegTest - mage goIntegTest`

Took 11 min 5 sec . View more details here
Description: mage goIntegTest

`Error signal`

Took 0 min 0 sec . View more details here
Description: Error 'hudson.AbortException: script returned exit code 1'

🐛 Flaky test report

❕ There are test failures but not known flaky tests.

Expand to view the summary

Genuine test errors

💔 There are test failures but not known flaky tests, most likely a genuine test failure.

Name: Build&Test / libbeat-unitTest / TestCollectPublishFailsNone – github.com/elastic/beats/v7/libbeat/outputs/elasticsearch
Name: Build&Test / libbeat-goIntegTest / TestClientBulkPublishEventsWithDeadletterIndex – github.com/elastic/beats/v7/libbeat/outputs/elasticsearch

🤖 GitHub comments

Expand to view the GitHub comments

To re-run your PR in the CI, just comment with:

/test : Re-trigger the build.
/package : Generate the packages and run the E2E tests.
/beats-tester : Run the installation tests with beats-tester.
run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

…cleanup

mergify · 2024-05-17T18:25:09Z

This pull request is now in conflicts. Could you fix it? 🙏
To fixup this pull request, you can check out it locally. See documentation: https://help.github.com/articles/checking-out-pull-requests-locally/

git fetch upstream
git checkout -b libbeat-metrics-cleanup upstream/libbeat-metrics-cleanup
git merge upstream/main
git push upstream libbeat-metrics-cleanup

…cleanup

mergify · 2024-05-25T00:03:11Z

This pull request is now in conflicts. Could you fix it? 🙏
To fixup this pull request, you can check out it locally. See documentation: https://help.github.com/articles/checking-out-pull-requests-locally/

git fetch upstream
git checkout -b libbeat-metrics-cleanup upstream/libbeat-metrics-cleanup
git merge upstream/main
git push upstream libbeat-metrics-cleanup

…cleanup

elasticmachine · 2024-05-25T00:10:51Z

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

cmacknz · 2024-05-27T15:48:17Z

libbeat/outputs/elasticsearch/client.go

+		// 409 is used to indicate an event with same ID already exists if
+		// `create` op_type is used.


Suggested change

// 409 is used to indicate an event with same ID already exists if

// `create` op_type is used.

// 409 is used to indicate an event with same ID already exists if

// `create` op_type is used.

You now also get a 409 if the TSDB dimensions are the same for two events, they form an implicit ID. This is probably the more common reason to hit this now.

fearful-symmetry · 2024-05-29T16:11:29Z

libbeat/outputs/elasticsearch/client.go

 const (
 	defaultEventType = "doc"
 )

+var bulkRequestParams = map[string]string{


Can we get some comments explaining this ES param? It's a bit odd without any context

cmacknz · 2024-06-03T20:30:33Z

libbeat/publisher/pipeline/monitoring.go

+			// which are also reported in events.dropped.
+			queueACKed: monitoring.NewUint(reg, "queue.acked"),
+
+			// (Gauge) queue.filled.pct.events measures the fraction (from 0 to 1)


Can we drop the .events from this, ahead of adding support for the queue size in bytes?

People will want to alert on this, they shouldn't have to define it twice.

Already part of #39774

…cleanup

… logs :(

Clean up / document metrics registry fields

8c3fc62

faec added bug cleanup Team:Elastic-Agent-Data-Plane Label for the Agent Data Plane team labels May 6, 2024

faec self-assigned this May 6, 2024

botelastic bot added needs_team Indicates that the issue/PR needs a Team:* label and removed needs_team Indicates that the issue/PR needs a Team:* label labels May 6, 2024

faec requested a review from fearful-symmetry May 6, 2024 13:06

faec added 3 commits May 16, 2024 16:22

Merge branch 'main' of github.com:elastic/beats into libbeat-metrics-…

5b0c0ad

…cleanup

fix tests

3583ff2

Add dead letter metric

5842ac2

faec added 6 commits May 17, 2024 18:00

reworking the ES publish helpers

6d00455

Merge branch 'main' of github.com:elastic/beats into libbeat-metrics-…

4345060

…cleanup

updating tests

0ae1785

working on tests

e53a90e

troubleshooting tests

3223bca

update / add tests

3ccb4f1

Merge branch 'main' of github.com:elastic/beats into libbeat-metrics-…

b1460c5

…cleanup

faec requested a review from belimawr May 25, 2024 00:10

faec marked this pull request as ready for review May 25, 2024 00:10

faec requested a review from a team as a code owner May 25, 2024 00:10

cmacknz reviewed May 27, 2024

View reviewed changes

fearful-symmetry reviewed May 29, 2024

View reviewed changes

faec added 2 commits May 29, 2024 16:06

review comments

c7b084b

lint

6a11ae0

belimawr approved these changes Jun 3, 2024

View reviewed changes

cmacknz reviewed Jun 3, 2024

View reviewed changes

faec added 5 commits June 5, 2024 17:44

event errors shouldn't break the connection

e95092f

Merge branch 'main' of github.com:elastic/beats into libbeat-metrics-…

74e0487

…cleanup

fix tests with undocumented dependencies on the text content of debug…

d7848fe

… logs :(

fix typo

bb4b14d

adjust flaky test

ee9e6fd

faec merged commit 37db599 into elastic:main Jun 6, 2024
105 of 108 checks passed

faec deleted the libbeat-metrics-cleanup branch June 6, 2024 18:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clean up / document metrics monitor fields #39413

Clean up / document metrics monitor fields #39413

faec commented May 6, 2024 •

edited

Loading

mergify bot commented May 6, 2024

elasticmachine commented May 6, 2024

Build stats

Test stats 🧪

`Build&Test / libbeat-unitTest / TestCollectPublishFailsNone – github.com/elastic/beats/v7/libbeat/outputs/elasticsearch`

`Build&Test / libbeat-goIntegTest / TestClientBulkPublishEventsWithDeadletterIndex – github.com/elastic/beats/v7/libbeat/outputs/elasticsearch`

`filebeat-goIntegTest - mage goIntegTest`

`libbeat-unitTest - mage build unitTest`

`libbeat-unitTest - mage build unitTest`

`libbeat-unitTest - mage build unitTest`

`libbeat-goIntegTest - mage goIntegTest`

`libbeat-goIntegTest - mage goIntegTest`

`libbeat-goIntegTest - mage goIntegTest`

`Error signal`

Genuine test errors

mergify bot commented May 17, 2024

mergify bot commented May 25, 2024

elasticmachine commented May 25, 2024

cmacknz May 27, 2024

faec May 29, 2024

fearful-symmetry May 29, 2024

faec May 29, 2024

cmacknz Jun 3, 2024

faec Jun 5, 2024

		// 409 is used to indicate an event with same ID already exists if
		// `create` op_type is used.

Clean up / document metrics monitor fields #39413

Clean up / document metrics monitor fields #39413

Conversation

faec commented May 6, 2024 • edited Loading

Checklist

Related issues

mergify bot commented May 6, 2024

elasticmachine commented May 6, 2024

💔 Tests Failed

Build stats

Test stats 🧪

Test errors

Build&Test / libbeat-unitTest / TestCollectPublishFailsNone – github.com/elastic/beats/v7/libbeat/outputs/elasticsearch

Build&Test / libbeat-goIntegTest / TestClientBulkPublishEventsWithDeadletterIndex – github.com/elastic/beats/v7/libbeat/outputs/elasticsearch

Steps errors

filebeat-goIntegTest - mage goIntegTest

libbeat-unitTest - mage build unitTest

libbeat-unitTest - mage build unitTest

libbeat-unitTest - mage build unitTest

libbeat-goIntegTest - mage goIntegTest

libbeat-goIntegTest - mage goIntegTest

libbeat-goIntegTest - mage goIntegTest

Error signal

🐛 Flaky test report

Genuine test errors

🤖 GitHub comments

mergify bot commented May 17, 2024

mergify bot commented May 25, 2024

elasticmachine commented May 25, 2024

cmacknz May 27, 2024

Choose a reason for hiding this comment

faec May 29, 2024

Choose a reason for hiding this comment

fearful-symmetry May 29, 2024

Choose a reason for hiding this comment

faec May 29, 2024

Choose a reason for hiding this comment

cmacknz Jun 3, 2024

Choose a reason for hiding this comment

faec Jun 5, 2024

Choose a reason for hiding this comment

faec commented May 6, 2024 •

edited

Loading

`Build&Test / libbeat-unitTest / TestCollectPublishFailsNone – github.com/elastic/beats/v7/libbeat/outputs/elasticsearch`

`Build&Test / libbeat-goIntegTest / TestClientBulkPublishEventsWithDeadletterIndex – github.com/elastic/beats/v7/libbeat/outputs/elasticsearch`

`filebeat-goIntegTest - mage goIntegTest`

`libbeat-unitTest - mage build unitTest`

`libbeat-unitTest - mage build unitTest`

`libbeat-unitTest - mage build unitTest`

`libbeat-goIntegTest - mage goIntegTest`

`libbeat-goIntegTest - mage goIntegTest`

`libbeat-goIntegTest - mage goIntegTest`

`Error signal`