libbeat/processors/cache: new processor #36619

efd6 · 2023-09-18T23:51:01Z

Proposed commit message

This is the initial infrastructure for a caching metadata processor to be added. It currently only supports in-memory caching, though both planned cache types are configurable with the file cache being mocked by an in-memory cache.

Additional config options are added relative to the RFC, but these are intentionally not documented at this stage.

This is part of the work required for elastic/integrations#2816.

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.

Author's Checklist

[ ]

How to test this PR locally

Related issues

Use cases

Screenshots

Logs

efd6 · 2023-09-19T00:39:44Z

run elasticsearch-ci/docs

elasticmachine · 2023-09-19T01:16:53Z

💚 Build Succeeded

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: 2023-09-25T10:04:47.935+0000
Duration: 72 min 14 sec

Test stats 🧪

Test	Results
Failed	0
Passed	28332
Skipped	2013
Total	30345

💚 Flaky test report

Tests succeeded.

🤖 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!)

efd6 · 2023-09-19T04:27:06Z

/package

efd6 · 2023-09-19T04:31:11Z

/test

elasticmachine · 2023-09-19T05:11:27Z

Pinging @elastic/security-external-integrations (Team:Security-External Integrations)

This is the initial infrastructure for a caching metadata processor to be added. It currently only supports in-memory caching, though both planned cache types are configurable with the file cache being mocked by an in-memory cache. Additional config options are added relative to the RFC, but these are intentionally not documented at this stage.

efd6 · 2023-09-19T06:22:02Z

golangci-lint appears to be ignoring the config on windows.

andrewkroh

This looks great. Left some minor comments.

andrewkroh · 2023-09-19T13:28:28Z

libbeat/processors/cache/_meta/fields.yml

I think this file is not required (especially since the processor doesn't have any of its own fields). Are there any issues if this is removed?

Seems to be ok locally. Trying a full build here.

andrewkroh · 2023-09-19T13:34:42Z

libbeat/processors/cache/cache.go

+	return s, func() {
+		// TODO: Consider making this reference counted.
+		// Currently, what we have is essentially an
+		// ownership model, where the put operation is


where the put operation is the owner

Or is it the first processor instantiated that is the owner?

You are correct. Will fix.

andrewkroh · 2023-09-19T13:54:23Z

libbeat/processors/cache/cache.go

+		// TODO: Consider making this reference counted.
+		// Currently, what we have is essentially an
+		// ownership model, where the put operation is
+		// owner. This could conceivably be problematic


With the flexibility afforded to users in their beat configurations and the fact that we allow for dynamic loading/reloading of configs in some areas, I would feel more confident with a reference count.

andrewkroh · 2023-09-19T14:03:30Z

libbeat/processors/cache/cache.go

+	// ... and write it into the cloned event.
+	result = event.Clone()
+	if _, err = result.PutValue(dst, meta); err != nil {


Is it necessary to clone? That is an expensive operation. I assume we want to clone so that if PutValue fails we don't leave the event in some partially modified state. Perhaps there are some pre-checks we can perform on the event to ensure that it never fails in PutValue?

That was the reason for it. I managed to significantly lower it to here, the point where the only failure possible is in the actual PutValue operation, and though this is not documented, the implementation of that fails before a mutation, so this should be fine. I'll change it and add a defensive test for the case.

andrewkroh

LGTM.

Can you please add an entry to the codeowners file for this package.

andrewkroh · 2023-09-20T19:29:47Z

libbeat/processors/cache/mem_store.go

+// backing data store is incomplete, and has no put operation defined, the TTL
+// will be invalid, but will never be accessed since all time operations outside
+// put refer to absolute times. setPutOptions also increases the reference count
+// for the memStore for all operation types.


setPutOptions also increases the reference count

That extra behavior was a little surprising. Consider an explicit "retain()" method?

Yeah, I was having similar thoughts. This is a consequence of the change to ref counting. I'm reluctant to make this two methods, but I think we get equivalent reduction in confusion by changing the name to add and removing the comment at the call site.

The whole ref count code is now significantly clearer.

rdner

LGTM except some typos and linter issues.

libbeat/processors/cache/mem_store.go

rdner · 2023-09-25T08:07:15Z

libbeat/processors/cache/mem_store_test.go

+			},
+			1: {
+				doTo: func(s *memStore) error {
+					s.Put("one", 1)


Suggested change

s.Put("one", 1)

_ = s.Put("one", 1)

~~This should not be flagged.~~

@efd6 this rule is for the github.com/elastic/elastic-agent-libs/mapstr.M type, right? And you have memStore.

Yeah, sorry, you are correct. I've added a nolint comment.

rdner · 2023-09-25T08:07:25Z

libbeat/processors/cache/mem_store_test.go

+			1: {
+				doTo: func(s *memStore) error {
+					s.Put("one", 1)
+					s.Put("two", 2)


Suggested change

s.Put("two", 2)

_ = s.Put("two", 2)

rdner · 2023-09-25T08:07:37Z

libbeat/processors/cache/mem_store_test.go

+				doTo: func(s *memStore) error {
+					s.Put("one", 1)
+					s.Put("two", 2)
+					s.Put("three", 3)


Suggested change

s.Put("three", 3)

_ = s.Put("three", 3)

This is the initial infrastructure for a caching metadata processor to be added. It currently only supports in-memory caching, though both planned cache types are configurable with the file cache being mocked by an in-memory cache. Additional config options are added relative to the RFC, but these are intentionally not documented at this stage.

efd6 added enhancement libbeat Team:Security-External Integrations backport-skip Skip notification from the automated backport with mergify 8.11-candidate labels Sep 18, 2023

efd6 self-assigned this Sep 18, 2023

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 Sep 18, 2023

efd6 force-pushed the i2816-cache_processor branch from b8b07d9 to 250243b Compare September 18, 2023 23:52

efd6 force-pushed the i2816-cache_processor branch 2 times, most recently from 8fd8793 to 42a6910 Compare September 19, 2023 01:09

efd6 marked this pull request as ready for review September 19, 2023 05:11

efd6 requested review from a team as code owners September 19, 2023 05:11

efd6 requested review from ycombinator and belimawr September 19, 2023 05:11

efd6 force-pushed the i2816-cache_processor branch 2 times, most recently from 01583da to 1aca826 Compare September 19, 2023 05:37

pierrehilbert added the Team:Elastic-Agent Label for the Agent team label Sep 19, 2023

efd6 force-pushed the i2816-cache_processor branch from 1aca826 to 6926e84 Compare September 19, 2023 05:44

efd6 force-pushed the i2816-cache_processor branch from 6926e84 to 4251d26 Compare September 19, 2023 05:52

andrewkroh reviewed Sep 19, 2023

View reviewed changes

efd6 added 2 commits September 20, 2023 07:59

address pr comments

f9e021c

drop fields definition

a39a793

efd6 force-pushed the i2816-cache_processor branch from 60a095d to a39a793 Compare September 19, 2023 22:35

efd6 requested a review from andrewkroh September 20, 2023 00:11

add check for invalid multiple backends

29a97e8

efd6 force-pushed the i2816-cache_processor branch 2 times, most recently from c25ce92 to fffd521 Compare September 20, 2023 06:48

improve logging

7c9f7eb

efd6 force-pushed the i2816-cache_processor branch from fffd521 to 7c9f7eb Compare September 20, 2023 10:18

andrewkroh approved these changes Sep 20, 2023

View reviewed changes

address pr comments

ac43bef

efd6 force-pushed the i2816-cache_processor branch from 73bfeda to ac43bef Compare September 25, 2023 07:38

rdner approved these changes Sep 25, 2023

View reviewed changes

address pr comments

27b0ce9

efd6 force-pushed the i2816-cache_processor branch from ddc0c59 to 27b0ce9 Compare September 25, 2023 10:04

efd6 merged commit f111cbc into elastic:main Sep 25, 2023
85 of 87 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

libbeat/processors/cache: new processor #36619

libbeat/processors/cache: new processor #36619

efd6 commented Sep 18, 2023 •

edited

Loading

efd6 commented Sep 19, 2023

elasticmachine commented Sep 19, 2023 •

edited by jenkins-beats-ci bot

Loading

Build stats

Test stats 🧪

efd6 commented Sep 19, 2023

efd6 commented Sep 19, 2023

elasticmachine commented Sep 19, 2023

efd6 commented Sep 19, 2023

andrewkroh left a comment •

edited

Loading

andrewkroh Sep 19, 2023

efd6 Sep 19, 2023

andrewkroh Sep 19, 2023

efd6 Sep 19, 2023

andrewkroh Sep 19, 2023

andrewkroh Sep 19, 2023

efd6 Sep 19, 2023 •

edited

Loading

andrewkroh left a comment

andrewkroh Sep 20, 2023

efd6 Sep 20, 2023 •

edited

Loading

efd6 Sep 20, 2023

rdner left a comment

rdner Sep 25, 2023

efd6 Sep 25, 2023 •

edited

Loading

rdner Sep 25, 2023

efd6 Sep 25, 2023

rdner Sep 25, 2023

rdner Sep 25, 2023

libbeat/processors/cache: new processor #36619

libbeat/processors/cache: new processor #36619

Conversation

efd6 commented Sep 18, 2023 • edited Loading

Proposed commit message

Checklist

Author's Checklist

How to test this PR locally

Related issues

Use cases

Screenshots

Logs

efd6 commented Sep 19, 2023

elasticmachine commented Sep 19, 2023 • edited by jenkins-beats-ci bot Loading

💚 Build Succeeded

Build stats

Test stats 🧪

💚 Flaky test report

🤖 GitHub comments

efd6 commented Sep 19, 2023

efd6 commented Sep 19, 2023

elasticmachine commented Sep 19, 2023

efd6 commented Sep 19, 2023

andrewkroh left a comment • edited Loading

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

Choose a reason for hiding this comment

efd6 Sep 19, 2023 • edited Loading

Choose a reason for hiding this comment

andrewkroh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

efd6 Sep 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rdner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

efd6 Sep 25, 2023 • edited Loading

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

efd6 commented Sep 18, 2023 •

edited

Loading

elasticmachine commented Sep 19, 2023 •

edited by jenkins-beats-ci bot

Loading

andrewkroh left a comment •

edited

Loading

efd6 Sep 19, 2023 •

edited

Loading

efd6 Sep 20, 2023 •

edited

Loading

efd6 Sep 25, 2023 •

edited

Loading