audit: Reduce the number of memory allocations in the hotpath #16056

graphcareful · 2024-01-10T21:15:30Z

With auditing enabled, high traffic events and low batch intervals it was observed that auditing was using more of the CPU then expected. This was traced back to many string conversions / string interpolation events when constructing an audit event.

An audit event is constructed at every possible attempt to enqueue the event so a hash code can be calculated. In the event the message has been determined it is a duplicate, the event can be deallocated, but this is wasteful. The event should only be constructed if it unique, to avoid the expensive serialization performed.

This pull request modifies the control flow in the audit log manager to calculate a hash code for the ocsf event without constructing the event itself. Since event construction depends on these parameters anyway, an appropriately unique hash code can be calculated with a free function using these parameters as input. In the event the hash code is unique, then the event will be allocated.

Linking to issue: #15898

Backports Required

Release Notes

Improvements

Reduces the number of allocations performed by the auditing subsystem

graphcareful · 2024-01-10T21:18:41Z

src/v/security/audit/schemas/hashing_utils.cc

+  ss::httpd::const_req req,
+  const ss::sstring& user,
+  const ss::sstring& svc_name) {
+    const auto activity_id = http_method_to_activity_id(req._method);


@michael-redpanda do we need the activity id to calculate the base hash? All that would matter is hashing the method I think. Then we could make this method private to utils.h again

This I believe is for the API activity, yes? API activity includes both HTTP and Kafka. Kafka doesn't have a 'method' field. So yes, I believe we do need to use activity ID in the hash. You should double check, but does "type_id" of the base type get hashed? If so then "activity_id" is already being hashed as Type ID is a combination of class ID and activity ID (https://schema.ocsf.io/1.0.0/classes/api_activity?extensions= look towards the bottom).

Yeah for sure, i was just wondering if it would be good enough to has something like 0 - 5, one value for each of the potential HTTP method types, instead of having to get one of 5 stringified versions of the enum to later just hash

graphcareful · 2024-01-10T21:21:18Z

I think it might also be worth it adding an additional find/replace commit to change the name of the equality_fields() family of methods. Currently they are now only used in the estimated_ocsf_size() method, I propose either changing the name of the method, or a small refactor to just have all ocsf types implement their own size() method (credit to Noah's original idea)

graphcareful · 2024-01-10T21:58:08Z

Changes in force-push

Fixed use after move
Fixed bug with not accounting for size sempahore units

src/v/security/audit/audit_log_manager.h

StephanDollberg

Thanks for looking into this.

Had a look and approach seems good. It seems like we are hashing quite a bit more now but maybe this is just an impression. Hashing was already quite visible before so want to make sure we are as smart as possible there.

Will leave to enterprise for a more thorough review.

graphcareful · 2024-01-11T13:00:42Z

Thanks for looking into this.

Had a look and approach seems good. It seems like we are hashing quite a bit more now but maybe this is just an impression. Hashing was already quite visible before so want to make sure we are as smart as possible there.

Will leave to enterprise for a more thorough review.

Thanks for taking a look! We should be hashing the same amount as in the current impl since the hash function is called at the top of do_enqueue_event just like it stands as of now in dev.

src/v/security/acl.h

src/v/security/audit/schemas/hashing_utils.h

src/v/security/audit/audit_log_manager.h

graphcareful · 2024-01-11T16:38:08Z

Changes in force-push

Added concept to constrain type T passed to thedo_enqueue method
Implemented Bens feedback making the hashing code more graceful
Fixed a bug in a hash method that returned a value of 0 that attributed to a few ducktape test failures

src/v/hashing/utils.h

src/v/security/audit/schemas/hashing_utils.cc

src/v/hashing/utils.h

graphcareful · 2024-01-11T21:03:47Z

Changes in force-push

Fix for bug that included product in the application lifecycle msg unconditionally, making a dt test fail

graphcareful · 2024-01-11T21:20:05Z

Change in force-push

Implemented Bens updated feedback on namespace around the new hashing utils and modifications to calls to hash_combine

graphcareful · 2024-01-11T21:57:14Z

Changes in force-push

Rebased against dev

vbotbuildovich · 2024-01-12T18:57:37Z

new failures in https://buildkite.com/redpanda/redpanda/builds/43691#018cfec8-1b3f-4b54-a5d6-8864b63b3835:

"rptest.tests.transactions_test.TransactionsAuthorizationTest.simple_authz_test"

src/v/hashing/utils.h

src/v/security/acl.h

src/v/hashing/utils.h

src/v/security/acl.h

src/v/hashing/utils.h

src/v/security/acl.h

- That way auditing can hash these types to quickly get a hash code of a larger ocsf type that encapsulates these acl options.

- This will be useful in future commits where a type can be passed to a templated method and static polymorphic dispatch can be used to determine which builder method to choose to construct the type

vbotbuildovich · 2024-01-18T13:13:32Z

/backport v23.3.x

graphcareful requested review from dotnwat, oleiman, StephanDollberg and michael-redpanda January 10, 2024 21:15

github-actions bot added the area/redpanda label Jan 10, 2024

graphcareful commented Jan 10, 2024

View reviewed changes

graphcareful force-pushed the audit-reduce-mem branch from 721cc98 to acabea1 Compare January 10, 2024 21:57

graphcareful commented Jan 10, 2024

View reviewed changes

src/v/security/audit/audit_log_manager.h Outdated Show resolved Hide resolved

StephanDollberg reviewed Jan 11, 2024

View reviewed changes

BenPope reviewed Jan 11, 2024

View reviewed changes

src/v/security/acl.h Outdated Show resolved Hide resolved

src/v/security/audit/schemas/hashing_utils.h Show resolved Hide resolved

BenPope reviewed Jan 11, 2024

View reviewed changes

src/v/security/audit/audit_log_manager.h Show resolved Hide resolved

graphcareful self-assigned this Jan 11, 2024

graphcareful force-pushed the audit-reduce-mem branch from acabea1 to ee41562 Compare January 11, 2024 16:36

BenPope reviewed Jan 11, 2024

View reviewed changes

graphcareful force-pushed the audit-reduce-mem branch from ee41562 to 28b117d Compare January 11, 2024 21:00

andrewhsu mentioned this pull request Jan 11, 2024

gha: use latest action for stale #16076

Merged

7 tasks

graphcareful force-pushed the audit-reduce-mem branch from 28b117d to c38c3d6 Compare January 11, 2024 21:19

graphcareful force-pushed the audit-reduce-mem branch from c38c3d6 to bed02c9 Compare January 11, 2024 21:56

graphcareful force-pushed the audit-reduce-mem branch from bed02c9 to 324f5d2 Compare January 11, 2024 22:05

graphcareful requested a review from BenPope January 16, 2024 15:46

michael-redpanda mentioned this pull request Jan 16, 2024

audit logging perf overhead #15898

Closed

BenPope reviewed Jan 16, 2024

View reviewed changes

src/v/hashing/utils.h Outdated Show resolved Hide resolved

src/v/security/acl.h Outdated Show resolved Hide resolved

graphcareful force-pushed the audit-reduce-mem branch from 324f5d2 to e893eca Compare January 16, 2024 18:59

graphcareful requested a review from BenPope January 16, 2024 20:02

dotnwat reviewed Jan 17, 2024

View reviewed changes

src/v/hashing/utils.h Outdated Show resolved Hide resolved

BenPope previously approved these changes Jan 17, 2024

View reviewed changes

src/v/security/acl.h Outdated Show resolved Hide resolved

BenPope reviewed Jan 17, 2024

View reviewed changes

src/v/hashing/utils.h Outdated Show resolved Hide resolved

BenPope reviewed Jan 17, 2024

View reviewed changes

src/v/security/acl.h Outdated Show resolved Hide resolved

graphcareful dismissed BenPope’s stale review via 0fa2d02 January 17, 2024 13:42

graphcareful force-pushed the audit-reduce-mem branch from e893eca to 0fa2d02 Compare January 17, 2024 13:42

graphcareful requested a review from BenPope January 17, 2024 13:42

graphcareful added 8 commits January 17, 2024 09:44

hashing: New utils for hashing multiple values

a1b4ffb

security: Make additional acl types hashable

89564aa

- That way auditing can hash these types to quickly get a hash code of a larger ocsf type that encapsulates these acl options.

s/audit: Remove unused method declaration

23628d0

s/audit: Remove unused method

01f32dd

s/audit: New family of methods to hash audit types

ef106e5

s/audit: Move ocsf builders to static methods

f197cad

- This will be useful in future commits where a type can be passed to a templated method and static polymorphic dispatch can be used to determine which builder method to choose to construct the type

s/audit: Use new hash mechanism for ocsf types

5340d6d

s/audit: Remove old now unused ocsf hash methods

8af1348

graphcareful force-pushed the audit-reduce-mem branch from 0fa2d02 to 8af1348 Compare January 17, 2024 14:44

BenPope approved these changes Jan 17, 2024

View reviewed changes

graphcareful merged commit 9be522f into redpanda-data:dev Jan 18, 2024
19 checks passed

vbotbuildovich mentioned this pull request Jan 18, 2024

[v23.3.x] audit: Reduce the number of memory allocations in the hotpath #16147

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

audit: Reduce the number of memory allocations in the hotpath #16056

audit: Reduce the number of memory allocations in the hotpath #16056

graphcareful commented Jan 10, 2024 •

edited

graphcareful Jan 10, 2024

michael-redpanda Jan 11, 2024

graphcareful Jan 11, 2024

graphcareful commented Jan 10, 2024

graphcareful commented Jan 10, 2024

StephanDollberg left a comment

graphcareful commented Jan 11, 2024

graphcareful commented Jan 11, 2024

graphcareful commented Jan 11, 2024

graphcareful commented Jan 11, 2024

graphcareful commented Jan 11, 2024

vbotbuildovich commented Jan 12, 2024

vbotbuildovich commented Jan 18, 2024

audit: Reduce the number of memory allocations in the hotpath #16056

audit: Reduce the number of memory allocations in the hotpath #16056

Conversation

graphcareful commented Jan 10, 2024 • edited

Backports Required

Release Notes

Improvements

graphcareful Jan 10, 2024

Choose a reason for hiding this comment

michael-redpanda Jan 11, 2024

Choose a reason for hiding this comment

graphcareful Jan 11, 2024

Choose a reason for hiding this comment

graphcareful commented Jan 10, 2024

graphcareful commented Jan 10, 2024

StephanDollberg left a comment

Choose a reason for hiding this comment

graphcareful commented Jan 11, 2024

graphcareful commented Jan 11, 2024

graphcareful commented Jan 11, 2024

graphcareful commented Jan 11, 2024

graphcareful commented Jan 11, 2024

vbotbuildovich commented Jan 12, 2024

vbotbuildovich commented Jan 18, 2024

graphcareful commented Jan 10, 2024 •

edited