Skip to content

feat(core): Make audit logger types extensible#3089

Open
elizabethhealy wants to merge 7 commits intomainfrom
dspx-2478-make-audit-logger-extensible
Open

feat(core): Make audit logger types extensible#3089
elizabethhealy wants to merge 7 commits intomainfrom
dspx-2478-make-audit-logger-extensible

Conversation

@elizabethhealy
Copy link
Copy Markdown
Member

@elizabethhealy elizabethhealy commented Feb 19, 2026

Proposed Changes

This pull request enhances the audit logging system by introducing extensibility for audit types and refactoring the event creation process. It allows dynamic registration of new object, action, and result types, and provides a cleaner API for generating audit events through dedicated Data Transfer Objects (DTOs). This change improves flexibility for future additions and better separates internal logging mechanisms from the public API.

Highlights

  • Extensible Audit Type Registration: Introduced new RegisterObjectType, RegisterActionType, and RegisterActionResult functions, allowing external modules to dynamically register new audit object types, action types, and action results. This enhances the extensibility of the audit logger without requiring core code modifications for new types.
  • Refactored Audit Event Structure: Refactored the internal audit event structures by introducing public Data Transfer Objects (DTOs) for EventMetaData, EventObjectObject, EventObjectAttributes, EventObjectAction, EventObjectActor, and EventClientInfo. These DTOs are now embedded within the internal event types, providing a clearer separation between internal logging mechanisms and the public API for event creation.
  • Simplified Audit Event Creation: Added a new NewEvent function that converts the public DTOs into the internal EventObject type, streamlining the process of creating audit events from external sources and improving the overall API for audit logging.

Checklist

  • I have added or updated unit tests
  • I have added or updated integration tests (if appropriate)
  • I have added or updated documentation

Testing Instructions

Summary by CodeRabbit

Release Notes

  • New Features

    • Added customizable audit event type registration system enabling runtime configuration of custom names for audit types
    • Introduced public API for audit event creation with structured parameters
    • Server startup now applies and seals audit type registrations
  • Tests

    • Added comprehensive test coverage for audit type registration and batch application functionality

@gemini-code-assist
Copy link
Copy Markdown
Contributor

Summary of Changes

Hello @elizabethhealy, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request significantly enhances the extensibility and maintainability of the audit logger. By introducing a mechanism for dynamically registering new audit types and refactoring the event creation process with dedicated DTOs, the system becomes more flexible for future additions and provides a cleaner, more robust API for generating audit events. This change allows for easier integration and expansion of audit logging capabilities across different services.

Highlights

  • Extensible Audit Type Registration: Introduced new RegisterObjectType, RegisterActionType, and RegisterActionResult functions, allowing external modules to dynamically register new audit object types, action types, and action results. This enhances the extensibility of the audit logger without requiring core code modifications for new types.
  • Refactored Audit Event Structure: Refactored the internal audit event structures by introducing public Data Transfer Objects (DTOs) for EventMetaData, EventObjectObject, EventObjectAttributes, EventObjectAction, EventObjectActor, and EventClientInfo. These DTOs are now embedded within the internal event types, providing a clearer separation between internal logging mechanisms and the public API for event creation.
  • Simplified Audit Event Creation: Added a new NewEvent function that converts the public DTOs into the internal EventObject type, streamlining the process of creating audit events from external sources and improving the overall API for audit logging.

🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console.

Changelog
  • service/logger/audit/constants.go
    • Added fmt and sync imports for new functionality.
    • Introduced auditTypeMu (a sync.RWMutex) to safely manage concurrent access to type name maps.
    • Replaced slice-based String() methods for ObjectType, ActionType, and ActionResult with map-based lookups for extensibility.
    • Added objectTypeNames, actionTypeNames, and actionResultNames maps to store string representations of audit types.
    • Implemented RegisterObjectType, RegisterActionType, and RegisterActionResult functions to allow dynamic registration of new audit type names.
  • service/logger/audit/getDecision.go
    • Updated CreateGetDecisionEvent and CreateV2GetDecisionEvent functions to use the new embedded DTOs for Attributes, Action, Actor, and ClientInfo fields.
  • service/logger/audit/getDecision_test.go
    • Modified test assertions in TestCreateGetDecisionEventHappyPathSuccess to align with the new embedded DTO structure for Object.Attributes, Action, Actor, and ClientInfo.
  • service/logger/audit/policy.go
    • Updated CreatePolicyEvent function to use the new embedded DTOs for Action, Actor, and ClientInfo fields.
  • service/logger/audit/policy_test.go
    • Modified test assertions in Test_CreatePolicyEvent_HappyPath to align with the new embedded DTO structure for Action, Actor, and ClientInfo.
  • service/logger/audit/rewrap.go
    • Updated CreateRewrapAuditEvent function to use the new embedded DTOs for Object.Attributes, Action, and Actor fields.
  • service/logger/audit/rewrap_test.go
    • Modified test assertions in TestCreateRewrapAuditEventHappyPath to align with the new embedded DTO structure for Object.Attributes, Action, and Actor.
  • service/logger/audit/utils.go
    • Defined new public DTO types: EventMetaData, EventObjectObject, EventObjectAttributes, EventObjectAction, EventObjectActor, EventClientInfo, and EventObjectParams.
    • Added a NewEvent function to convert EventObjectParams (public DTOs) into the internal EventObject structure.
    • Refactored internal eventObjectAttributes, eventAction, auditEventActor, and eventClientInfo structs to embed their corresponding new public DTOs.
Activity
  • No specific activity (comments, reviews, or progress updates) has been recorded for this pull request yet.
Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help /gemini help Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.


With types now open wide, Audit logs can grow and stride, Go's mutex guards, Against concurrent hazards, New events, with grace, now glide.

Footnotes

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces a significant and well-structured refactoring to make the audit logger extensible. The key changes include allowing dynamic registration of audit types (ObjectType, ActionType, ActionResult) and defining a clear public DTO layer for creating audit events, which decouples consumers from internal logger implementation details. My review focuses on two main areas for improvement: enhancing concurrency by using more granular locking for the new type registries, and consistently applying the new NewEvent helper function across the codebase to improve maintainability and code clarity.

Comment thread service/logger/audit/constants.go Outdated
@github-actions
Copy link
Copy Markdown
Contributor

Benchmark results, click to expand

Benchmark authorization.GetDecisions Results:

Metric Value
Approved Decision Requests 1000
Denied Decision Requests 0
Total Time 182.442637ms

Benchmark authorization.v2.GetMultiResourceDecision Results:

Metric Value
Approved Decision Requests 1000
Denied Decision Requests 0
Total Time 98.371262ms

Benchmark Statistics

Name № Requests Avg Duration Min Duration Max Duration

Bulk Benchmark Results

Metric Value
Total Decrypts 100
Successful Decrypts 100
Failed Decrypts 0
Total Time 373.222902ms
Throughput 267.94 requests/second

TDF3 Benchmark Results:

Metric Value
Total Requests 5000
Successful Requests 5000
Failed Requests 0
Concurrent Requests 50
Total Time 39.083798779s
Average Latency 388.741613ms
Throughput 127.93 requests/second

@github-actions
Copy link
Copy Markdown
Contributor

@github-actions
Copy link
Copy Markdown
Contributor

Benchmark results, click to expand

Benchmark authorization.GetDecisions Results:

Metric Value
Approved Decision Requests 1000
Denied Decision Requests 0
Total Time 209.291218ms

Benchmark authorization.v2.GetMultiResourceDecision Results:

Metric Value
Approved Decision Requests 1000
Denied Decision Requests 0
Total Time 103.345629ms

Benchmark Statistics

Name № Requests Avg Duration Min Duration Max Duration

Bulk Benchmark Results

Metric Value
Total Decrypts 100
Successful Decrypts 100
Failed Decrypts 0
Total Time 377.514601ms
Throughput 264.89 requests/second

TDF3 Benchmark Results:

Metric Value
Total Requests 5000
Successful Requests 5000
Failed Requests 0
Concurrent Requests 50
Total Time 41.969693982s
Average Latency 417.123519ms
Throughput 119.13 requests/second

@github-actions
Copy link
Copy Markdown
Contributor

@elizabethhealy elizabethhealy marked this pull request as ready for review February 19, 2026 18:12
@elizabethhealy elizabethhealy requested a review from a team as a code owner February 19, 2026 18:12
cshamrick
cshamrick previously approved these changes Feb 19, 2026
jakedoublev
jakedoublev previously approved these changes Feb 19, 2026
Copy link
Copy Markdown
Contributor

@jakedoublev jakedoublev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These changes make sense to allow upstream registered PEPs and services registered to the platform but not present within opentdf to log to audit. These changes are an incremental improvement and do not seem too difficult to walk back as the audit story evolves.

pflynn-virtru
pflynn-virtru previously approved these changes Feb 19, 2026
@elizabethhealy elizabethhealy changed the title feat(core): Make audit logger extensible feat(core): Make audit logger types extensible Mar 6, 2026
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Mar 6, 2026

Benchmark results, click to expand

Benchmark authorization.GetDecisions Results:

Metric Value
Approved Decision Requests 1000
Denied Decision Requests 0
Total Time 209.426508ms

Benchmark authorization.v2.GetMultiResourceDecision Results:

Metric Value
Approved Decision Requests 1000
Denied Decision Requests 0
Total Time 128.839597ms

Benchmark Statistics

Name № Requests Avg Duration Min Duration Max Duration

Bulk Benchmark Results

Metric Value
Total Decrypts 100
Successful Decrypts 100
Failed Decrypts 0
Total Time 391.362353ms
Throughput 255.52 requests/second

TDF3 Benchmark Results:

Metric Value
Total Requests 5000
Successful Requests 5000
Failed Requests 0
Concurrent Requests 50
Total Time 42.644577678s
Average Latency 424.996368ms
Throughput 117.25 requests/second

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Mar 6, 2026

X-Test Failure Report

opentdfplatformW2AMTG.dockerbuild

@elizabethhealy
Copy link
Copy Markdown
Member Author

/gemini review

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Mar 6, 2026

Benchmark results, click to expand

Benchmark authorization.GetDecisions Results:

Metric Value
Approved Decision Requests 1000
Denied Decision Requests 0
Total Time 186.499546ms

Benchmark authorization.v2.GetMultiResourceDecision Results:

Metric Value
Approved Decision Requests 1000
Denied Decision Requests 0
Total Time 98.389429ms

Benchmark Statistics

Name № Requests Avg Duration Min Duration Max Duration

Bulk Benchmark Results

Metric Value
Total Decrypts 100
Successful Decrypts 100
Failed Decrypts 0
Total Time 378.136581ms
Throughput 264.45 requests/second

TDF3 Benchmark Results:

Metric Value
Total Requests 5000
Successful Requests 5000
Failed Requests 0
Concurrent Requests 50
Total Time 39.780081066s
Average Latency 395.291085ms
Throughput 125.69 requests/second

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces a valuable enhancement by making the audit logger types extensible and refactoring the event creation process with DTOs. This improves flexibility and provides a cleaner API. My review focuses on a potential side-effect in one of the new option functions, opportunities for code simplification, and promoting consistency by using the new NewEvent helper function across the module. Addressing these points will further improve the robustness and maintainability of the new implementation.

Comment thread service/pkg/server/options.go
Comment thread service/logger/audit/getDecision.go
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Mar 6, 2026

@elizabethhealy
Copy link
Copy Markdown
Member Author

/gemini summary

@gemini-code-assist
Copy link
Copy Markdown
Contributor

Summary of Changes

This pull request significantly enhances the audit logging system by introducing extensibility for audit types and refactoring the event creation process. It allows dynamic registration of new object, action, and result types, and provides a cleaner API for generating audit events through dedicated Data Transfer Objects (DTOs). This change improves flexibility for future additions and better separates internal logging mechanisms from the public API.

Highlights

  • Extensible Audit Type Registration: Introduced new RegisterObjectType, RegisterActionType, and RegisterActionResult functions, allowing external modules to dynamically register new audit object types, action types, and action results. This enhances the extensibility of the audit logger without requiring core code modifications for new types.
  • Refactored Audit Event Structure: Refactored the internal audit event structures by introducing public Data Transfer Objects (DTOs) for EventMetaData, EventObjectObject, EventObjectAttributes, EventObjectAction, EventObjectActor, and EventClientInfo. These DTOs are now embedded within the internal event types, providing a clearer separation between internal logging mechanisms and the public API for event creation.
  • Simplified Audit Event Creation: Added a new NewEvent function that converts the public DTOs into the internal EventObject type, streamlining the process of creating audit events from external sources and improving the overall API for audit logging.

🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console.

Changelog
  • service/logger/audit/constants.go
    • Added errors, fmt, and sync imports.
    • Introduced ErrAuditTypeRegistrationSealed error.
    • Replaced slice-based String() methods for ObjectType, ActionType, and ActionResult with map-based lookups for dynamic type names.
    • Added objectTypeNames, actionTypeNames, and actionResultNames maps to store string representations of audit types.
    • Introduced auditTypeRegistryMu (a sync.RWMutex) and typeRegistrationSealed boolean to manage concurrent access and prevent modifications after sealing.
    • Implemented RegisterObjectType, RegisterActionType, and RegisterActionResult functions for dynamic registration of new audit type names, with checks for sealed registrations.
    • Added TypeRegistrations struct to group all type maps.
    • Added ApplyTypeRegistrations function to apply a batch of registrations.
    • Added SealTypeRegistrations function to prevent further type registrations.
  • service/logger/audit/constants_test.go
    • Added a new test file to verify the functionality of RegisterObjectType, RegisterActionType, RegisterActionResult, ApplyTypeRegistrations, and SealTypeRegistrations.
  • service/logger/audit/getDecision.go
    • Updated CreateGetDecisionEvent and CreateV2GetDecisionEvent functions to use the new embedded DTOs (EventObjectAttributes, EventObjectAction, EventObjectActor, EventClientInfo) within their respective internal event structures.
  • service/logger/audit/getDecision_test.go
    • Modified test assertions in TestCreateGetDecisionEventHappyPathSuccess to reflect the new embedded DTO structure for Object.Attributes, Action, Actor, and ClientInfo.
  • service/logger/audit/policy.go
    • Updated CreatePolicyEvent function to use the new embedded DTOs (EventObjectAction, EventObjectActor, EventClientInfo) within its internal event structures.
  • service/logger/audit/policy_test.go
    • Modified test assertions in Test_CreatePolicyEvent_HappyPath to reflect the new embedded DTO structure for Action, Actor, and ClientInfo.
  • service/logger/audit/rewrap.go
    • Updated CreateRewrapAuditEvent function to use the new embedded DTOs (EventObjectAttributes, EventObjectAction, EventObjectActor, EventClientInfo) within its internal event structures.
  • service/logger/audit/rewrap_test.go
    • Modified test assertions in TestCreateRewrapAuditEventHappyPath to reflect the new embedded DTO structure for Object.Attributes, Action, Actor, and ClientInfo.
  • service/logger/audit/utils.go
    • Introduced new public DTO types: EventMetaData, EventObjectObject, EventObjectAttributes, EventObjectAction, EventObjectActor, EventClientInfo, and EventObjectParams.
    • Added a NewEvent function to convert EventObjectParams (public DTOs) into the internal EventObject structure.
    • Refactored internal eventObjectAttributes, eventAction, auditEventActor, and eventClientInfo structs to embed their corresponding new public DTOs.
  • service/pkg/server/options.go
    • Added auditTypeRegistrations field to StartConfig to hold audit.TypeRegistrations.
    • Implemented WithAdditionalAuditTypeRegistrations option to merge and store additional audit type registrations.
  • service/pkg/server/options_test.go
    • Added TestWithAdditionalAuditTypeRegistrations to verify the merging logic of audit type registrations.
  • service/pkg/server/start.go
    • Imported github.com/opentdf/platform/service/logger/audit.
    • Added logic to apply startConfig.auditTypeRegistrations and then SealTypeRegistrations during server startup.
Activity
  • February 19, 2026: gemini-code-assist[bot] provided an initial summary of changes and highlights.
  • February 19, 2026: github-actions[bot] posted benchmark results.
  • February 19, 2026: github-actions[bot] reported X-Test failures.
  • February 19, 2026: github-actions[bot] posted updated benchmark results.
  • February 19, 2026: github-actions[bot] reported X-Test results, showing success for js, java, and go tests.
  • February 19, 2026: gemini-code-assist[bot] provided a review comment suggesting separate mutexes for each audit type map to reduce lock contention.
  • March 6, 2026: github-actions[bot] posted benchmark results.
  • March 6, 2026: github-actions[bot] reported X-Test failures.
  • March 6, 2026: elizabethhealy requested a /gemini review.
  • March 6, 2026: github-actions[bot] posted benchmark results.
  • March 6, 2026: gemini-code-assist[bot] provided a review comment suggesting to avoid modifying input arguments in WithAdditionalAuditTypeRegistrations by creating new maps for merged results.
  • March 6, 2026: gemini-code-assist[bot] provided a review comment suggesting a more concise struct literal for embedded structs.
  • March 6, 2026: elizabethhealy responded to the concise struct literal suggestion, stating it's "not supported by the current go toolchain version".
  • March 6, 2026: github-actions[bot] reported X-Test results, showing success for go, java, and js tests.
  • March 6, 2026: elizabethhealy requested a /gemini summary.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Mar 6, 2026

Benchmark results, click to expand

Benchmark authorization.GetDecisions Results:

Metric Value
Approved Decision Requests 1000
Denied Decision Requests 0
Total Time 212.671483ms

Benchmark authorization.v2.GetMultiResourceDecision Results:

Metric Value
Approved Decision Requests 1000
Denied Decision Requests 0
Total Time 104.398027ms

Benchmark Statistics

Name № Requests Avg Duration Min Duration Max Duration

Bulk Benchmark Results

Metric Value
Total Decrypts 100
Successful Decrypts 100
Failed Decrypts 0
Total Time 385.195793ms
Throughput 259.61 requests/second

TDF3 Benchmark Results:

Metric Value
Total Requests 5000
Successful Requests 5000
Failed Requests 0
Concurrent Requests 50
Total Time 42.042136333s
Average Latency 418.591298ms
Throughput 118.93 requests/second

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Mar 6, 2026

@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Apr 2, 2026

📝 Walkthrough

Walkthrough

Introduces a pluggable audit type registration system with thread-safe registration and sealing mechanisms, creates public DTOs for audit events, refactors event creation functions to use nested wrapper fields, and integrates type registrations into server startup flow.

Changes

Cohort / File(s) Summary
Audit Type Registration System
service/logger/audit/constants.go, service/logger/audit/constants_test.go
Replaces hardcoded string implementations with mutable name maps for ObjectType, ActionType, and ActionResult. Adds thread-safe registration functions (RegisterObjectType, RegisterActionType, RegisterActionResult, ApplyTypeRegistrations, SealTypeRegistrations) guarded by an RWMutex with sealing capability. Includes comprehensive test coverage for registration, batch application, and sealed-state behavior.
Event Structure Refactoring
service/logger/audit/getDecision.go, service/logger/audit/getDecision_test.go, service/logger/audit/policy.go, service/logger/audit/policy_test.go, service/logger/audit/rewrap.go, service/logger/audit/rewrap_test.go
Updates event creation functions to wrap previously top-level fields (Action, Actor, ClientInfo, ObjectAttributes) into nested wrapper structs. All event-building logic and control flow remain unchanged; only field nesting structure is updated across tests and implementations.
Public DTOs & Utilities
service/logger/audit/utils.go
Introduces exported public DTOs (EventObjectObject, EventObjectAttributes, EventObjectAction, EventObjectActor, EventClientInfo, EventObjectParams, EventMetaData) and refactors internal nested structs to embed corresponding public DTO types. Adds NewEvent(params EventObjectParams) *EventObject constructor for downstream consumers.
Server Configuration & Integration
service/pkg/server/options.go, service/pkg/server/options_test.go, service/pkg/server/start.go
Adds auditTypeRegistrations field to StartConfig and new option function WithAdditionalAuditTypeRegistrations() for merging audit type mappings. Integrates type registration application and sealing into server startup flow after logger/tracer setup.

Sequence Diagram

sequenceDiagram
    participant Server as Server Startup
    participant Registrar as Audit Registry
    participant Events as Event Creation
    
    Server->>Registrar: ApplyTypeRegistrations(TypeRegistrations)
    Note over Registrar: Register ObjectType,<br/>ActionType, ActionResult names
    Registrar-->>Server: Registration applied
    
    Server->>Registrar: SealTypeRegistrations()
    Note over Registrar: Lock registry,<br/>prevent new registrations
    Registrar-->>Server: Sealed
    
    Server->>Events: Create events (GetDecision, Policy, Rewrap)
    Events->>Registrar: ObjectType.String()
    Registrar-->>Events: Return registered name<br/>or fallback format
    Events-->>Server: EventObject with<br/>custom type names
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~55 minutes

Poem

🐰 Registry grows with names so bright,
Thread-safe locks hold types tight,
Events nest in wrapper's care,
Public DTOs now laid bare,
Sealed and ready, all just right! ✨

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 16.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'feat(core): Make audit logger types extensible' accurately describes the main change: introducing extensibility for the audit logging system through registration functions and public DTOs.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch dspx-2478-make-audit-logger-extensible

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 2, 2026

Benchmark results, click to expand

Benchmark authorization.GetDecisions Results:

Metric Value
Approved Decision Requests 1000
Denied Decision Requests 0
Total Time 191.438365ms

Benchmark authorization.v2.GetMultiResourceDecision Results:

Metric Value
Approved Decision Requests 1000
Denied Decision Requests 0
Total Time 96.338264ms

Benchmark Statistics

Name № Requests Avg Duration Min Duration Max Duration

Bulk Benchmark Results

Metric Value
Total Decrypts 100
Successful Decrypts 100
Failed Decrypts 0
Total Time 385.629226ms
Throughput 259.32 requests/second

TDF3 Benchmark Results:

Metric Value
Total Requests 5000
Successful Requests 5000
Failed Requests 0
Concurrent Requests 50
Total Time 40.140543863s
Average Latency 399.389898ms
Throughput 124.56 requests/second

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 5

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@service/logger/audit/constants_test.go`:
- Around line 10-16: The helper resetAuditTypeRegistrationState currently only
resets typeRegistrationSealed; update it to also clear or reinitialize the
global registration maps objectTypeNames, actionTypeNames, and actionResultNames
while holding auditTypeRegistryMu to avoid races, and consider using t.Cleanup
in tests that register types to unregister specific entries; ensure
auditTypeRegistryMu.Lock() is held before mutating these maps and unlocked
afterwards so the global state is fully reset between tests.

In `@service/logger/audit/constants.go`:
- Around line 89-97: RegisterObjectType currently allows empty names which can
break logs; update RegisterObjectType to validate that the provided name is
non-empty (return a suitable error, e.g., ErrInvalidName) before acquiring or
after checking typeRegistrationSealed, and only set objectTypeNames[ot]=name
when name is non-empty; apply the same validation pattern to RegisterActionType
and RegisterActionResult so all registry functions reject empty strings and
return a clear error instead of registering an empty name.
- Around line 197-217: ApplyTypeRegistrations may partially apply registrations
if the registry is sealed mid-run or a future validation fails; before iterating
over reg.ObjectTypes/reg.ActionTypes/reg.ActionResults, check the registry
sealed state once (e.g., call the existing IsSealed or equivalent) and return an
error if sealed so no partial changes occur. In other words, at the start of
ApplyTypeRegistrations validate the registry is writable, then proceed to call
RegisterObjectType, RegisterActionType, and RegisterActionResult for each entry;
this ensures either all supplied registrations are attempted or none are applied
when the registry cannot be modified.

In `@service/pkg/server/options.go`:
- Around line 269-305: The merge in WithAdditionalAuditTypeRegistrations
silently overwrites earlier registrations (StartConfig.auditTypeRegistrations)
when keys collide; instead, detect conflicting keys (for ObjectTypes,
ActionTypes, ActionResults) and surface them rather than making order
matter—either keep the batches separate by appending the incoming registrations
into a pending bucket on StartConfig (e.g.,
StartConfig.pendingAuditTypeRegistrations / additionalAuditTypeRegistrations)
for validation during Start(), or immediately record conflicts into a new
StartConfig.auditTypeRegistrationConflicts slice (include the conflicting key
and both names) so Start() can fail with a clear error; update
WithAdditionalAuditTypeRegistrations to compare keys from
c.auditTypeRegistrations and registrations, avoid silent overwrites, and
populate the chosen pending/conflict fields accordingly.

In `@service/pkg/server/start.go`:
- Around line 130-134: The code calls audit.SealTypeRegistrations() inside
Start(), making the package-global registry permanently sealed and preventing
retries; change this by deferring sealing until after Start completes
successfully (move the audit.SealTypeRegistrations() call out of the error path
and only invoke it after all startup steps succeed), or refactor to avoid global
state by introducing a registry tied to the server instance (e.g., replace
package-global sealing with an instance method on a registry object
created/owned by Start and initialized via
audit.ApplyTypeRegistrations(startConfig.auditTypeRegistrations)), ensuring
audit.ApplyTypeRegistrations can still be called on retries before any seal is
applied.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 6eb9d82d-428c-480d-bf14-9ee518b37b52

📥 Commits

Reviewing files that changed from the base of the PR and between f04a385 and c9068a1.

📒 Files selected for processing (12)
  • service/logger/audit/constants.go
  • service/logger/audit/constants_test.go
  • service/logger/audit/getDecision.go
  • service/logger/audit/getDecision_test.go
  • service/logger/audit/policy.go
  • service/logger/audit/policy_test.go
  • service/logger/audit/rewrap.go
  • service/logger/audit/rewrap_test.go
  • service/logger/audit/utils.go
  • service/pkg/server/options.go
  • service/pkg/server/options_test.go
  • service/pkg/server/start.go

Comment on lines +10 to +16
func resetAuditTypeRegistrationState(t *testing.T) {
t.Helper()

auditTypeRegistryMu.Lock()
typeRegistrationSealed = false
auditTypeRegistryMu.Unlock()
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Incomplete reset: registered types persist between tests.

The resetAuditTypeRegistrationState helper only resets the typeRegistrationSealed flag but doesn't clear the custom types registered in objectTypeNames, actionTypeNames, and actionResultNames maps. This leads to:

  1. Test pollution: Custom types registered in earlier tests remain in global maps
  2. Potential false positives: A test could pass because a type was registered by a previous test
  3. Memory accumulation: In long test runs, orphaned registrations accumulate

Consider also resetting the maps to their initial state or using t.Cleanup to remove registered test types.

🛠️ Proposed fix with map cleanup
 func resetAuditTypeRegistrationState(t *testing.T) {
 	t.Helper()
 
 	auditTypeRegistryMu.Lock()
+	// Remove any previously registered test types (those with IDs >= 10000)
+	for k := range objectTypeNames {
+		if k >= 10000 {
+			delete(objectTypeNames, k)
+		}
+	}
+	for k := range actionTypeNames {
+		if k >= 10000 {
+			delete(actionTypeNames, k)
+		}
+	}
+	for k := range actionResultNames {
+		if k >= 10000 {
+			delete(actionResultNames, k)
+		}
+	}
 	typeRegistrationSealed = false
 	auditTypeRegistryMu.Unlock()
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
func resetAuditTypeRegistrationState(t *testing.T) {
t.Helper()
auditTypeRegistryMu.Lock()
typeRegistrationSealed = false
auditTypeRegistryMu.Unlock()
}
func resetAuditTypeRegistrationState(t *testing.T) {
t.Helper()
auditTypeRegistryMu.Lock()
// Remove any previously registered test types (those with IDs >= 10000)
for k := range objectTypeNames {
if k >= 10000 {
delete(objectTypeNames, k)
}
}
for k := range actionTypeNames {
if k >= 10000 {
delete(actionTypeNames, k)
}
}
for k := range actionResultNames {
if k >= 10000 {
delete(actionResultNames, k)
}
}
typeRegistrationSealed = false
auditTypeRegistryMu.Unlock()
}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@service/logger/audit/constants_test.go` around lines 10 - 16, The helper
resetAuditTypeRegistrationState currently only resets typeRegistrationSealed;
update it to also clear or reinitialize the global registration maps
objectTypeNames, actionTypeNames, and actionResultNames while holding
auditTypeRegistryMu to avoid races, and consider using t.Cleanup in tests that
register types to unregister specific entries; ensure auditTypeRegistryMu.Lock()
is held before mutating these maps and unlocked afterwards so the global state
is fully reset between tests.

Comment on lines +89 to +97
func RegisterObjectType(ot ObjectType, name string) error {
auditTypeRegistryMu.Lock()
defer auditTypeRegistryMu.Unlock()
if typeRegistrationSealed {
return ErrAuditTypeRegistrationSealed
}
objectTypeNames[ot] = name
return nil
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial

Consider validating that name is non-empty.

The Register* functions accept any string, including empty strings. An empty name would produce empty strings in audit logs, potentially breaking log parsing or monitoring.

♻️ Optional: Add name validation
 func RegisterObjectType(ot ObjectType, name string) error {
+	if name == "" {
+		return errors.New("audit type name cannot be empty")
+	}
 	auditTypeRegistryMu.Lock()
 	defer auditTypeRegistryMu.Unlock()
 	if typeRegistrationSealed {
 		return ErrAuditTypeRegistrationSealed
 	}
 	objectTypeNames[ot] = name
 	return nil
 }

Apply the same pattern to RegisterActionType and RegisterActionResult.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
func RegisterObjectType(ot ObjectType, name string) error {
auditTypeRegistryMu.Lock()
defer auditTypeRegistryMu.Unlock()
if typeRegistrationSealed {
return ErrAuditTypeRegistrationSealed
}
objectTypeNames[ot] = name
return nil
}
func RegisterObjectType(ot ObjectType, name string) error {
if name == "" {
return errors.New("audit type name cannot be empty")
}
auditTypeRegistryMu.Lock()
defer auditTypeRegistryMu.Unlock()
if typeRegistrationSealed {
return ErrAuditTypeRegistrationSealed
}
objectTypeNames[ot] = name
return nil
}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@service/logger/audit/constants.go` around lines 89 - 97, RegisterObjectType
currently allows empty names which can break logs; update RegisterObjectType to
validate that the provided name is non-empty (return a suitable error, e.g.,
ErrInvalidName) before acquiring or after checking typeRegistrationSealed, and
only set objectTypeNames[ot]=name when name is non-empty; apply the same
validation pattern to RegisterActionType and RegisterActionResult so all
registry functions reject empty strings and return a clear error instead of
registering an empty name.

Comment on lines +197 to +217
func ApplyTypeRegistrations(reg TypeRegistrations) error {
for objectType, name := range reg.ObjectTypes {
if err := RegisterObjectType(objectType, name); err != nil {
return err
}
}

for actionType, name := range reg.ActionTypes {
if err := RegisterActionType(actionType, name); err != nil {
return err
}
}

for actionResult, name := range reg.ActionResults {
if err := RegisterActionResult(actionResult, name); err != nil {
return err
}
}

return nil
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial

ApplyTypeRegistrations can leave registry in partially applied state on failure.

If the registry becomes sealed mid-iteration (unlikely in single-threaded startup) or if a future validation is added that can fail, some registrations will have succeeded while others didn't. The caller has no way to know which succeeded.

Given that this is called at startup before sealing (per start.go), this is currently safe. However, for robustness, consider checking sealed state once upfront before any modifications.

♻️ Optional: Check sealed state upfront
 func ApplyTypeRegistrations(reg TypeRegistrations) error {
+	auditTypeRegistryMu.RLock()
+	if typeRegistrationSealed {
+		auditTypeRegistryMu.RUnlock()
+		return ErrAuditTypeRegistrationSealed
+	}
+	auditTypeRegistryMu.RUnlock()
+
 	for objectType, name := range reg.ObjectTypes {
 		if err := RegisterObjectType(objectType, name); err != nil {
 			return err
 		}
 	}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@service/logger/audit/constants.go` around lines 197 - 217,
ApplyTypeRegistrations may partially apply registrations if the registry is
sealed mid-run or a future validation fails; before iterating over
reg.ObjectTypes/reg.ActionTypes/reg.ActionResults, check the registry sealed
state once (e.g., call the existing IsSealed or equivalent) and return an error
if sealed so no partial changes occur. In other words, at the start of
ApplyTypeRegistrations validate the registry is writable, then proceed to call
RegisterObjectType, RegisterActionType, and RegisterActionResult for each entry;
this ensures either all supplied registrations are attempted or none are applied
when the registry cannot be modified.

Comment on lines +269 to +305
func WithAdditionalAuditTypeRegistrations(registrations audit.TypeRegistrations) StartOptions {
return func(c StartConfig) StartConfig {
mergedRegistrations := audit.TypeRegistrations{}

if len(c.auditTypeRegistrations.ObjectTypes) > 0 || len(registrations.ObjectTypes) > 0 {
mergedRegistrations.ObjectTypes = make(map[audit.ObjectType]string)
for objectType, name := range c.auditTypeRegistrations.ObjectTypes {
mergedRegistrations.ObjectTypes[objectType] = name
}
for objectType, name := range registrations.ObjectTypes {
mergedRegistrations.ObjectTypes[objectType] = name
}
}

if len(c.auditTypeRegistrations.ActionTypes) > 0 || len(registrations.ActionTypes) > 0 {
mergedRegistrations.ActionTypes = make(map[audit.ActionType]string)
for actionType, name := range c.auditTypeRegistrations.ActionTypes {
mergedRegistrations.ActionTypes[actionType] = name
}
for actionType, name := range registrations.ActionTypes {
mergedRegistrations.ActionTypes[actionType] = name
}
}

if len(c.auditTypeRegistrations.ActionResults) > 0 || len(registrations.ActionResults) > 0 {
mergedRegistrations.ActionResults = make(map[audit.ActionResult]string)
for actionResult, name := range c.auditTypeRegistrations.ActionResults {
mergedRegistrations.ActionResults[actionResult] = name
}
for actionResult, name := range registrations.ActionResults {
mergedRegistrations.ActionResults[actionResult] = name
}
}

c.auditTypeRegistrations = mergedRegistrations
return c
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Surface registration collisions instead of making them order-dependent.

If two extensions provide the same ObjectType/ActionType/ActionResult, this merge silently keeps the later name and discards the earlier one. That makes the emitted audit taxonomy depend on option order instead of surfacing a duplicate-registration error; keep the batches separate until Start() or explicitly detect conflicting keys here.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@service/pkg/server/options.go` around lines 269 - 305, The merge in
WithAdditionalAuditTypeRegistrations silently overwrites earlier registrations
(StartConfig.auditTypeRegistrations) when keys collide; instead, detect
conflicting keys (for ObjectTypes, ActionTypes, ActionResults) and surface them
rather than making order matter—either keep the batches separate by appending
the incoming registrations into a pending bucket on StartConfig (e.g.,
StartConfig.pendingAuditTypeRegistrations / additionalAuditTypeRegistrations)
for validation during Start(), or immediately record conflicts into a new
StartConfig.auditTypeRegistrationConflicts slice (include the conflicting key
and both names) so Start() can fail with a clear error; update
WithAdditionalAuditTypeRegistrations to compare keys from
c.auditTypeRegistrations and registrations, avoid silent overwrites, and
populate the chosen pending/conflict fields accordingly.

Comment on lines +130 to +134
if err := audit.ApplyTypeRegistrations(startConfig.auditTypeRegistrations); err != nil {
return fmt.Errorf("failed applying additional audit type registrations: %w", err)
}
audit.SealTypeRegistrations()
logger.Debug("sealed audit type registrations")
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Sealing the registry here makes Start non-retryable.

SealTypeRegistrations flips package-global state with no production unseal path in service/logger/audit/constants.go. Once Line 133 runs, any later Start() in the same process cannot apply additional registrations, and any failure later in startup still leaves the process permanently sealed. Seal only after startup succeeds, or tie the registry lifetime to the server instance.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@service/pkg/server/start.go` around lines 130 - 134, The code calls
audit.SealTypeRegistrations() inside Start(), making the package-global registry
permanently sealed and preventing retries; change this by deferring sealing
until after Start completes successfully (move the audit.SealTypeRegistrations()
call out of the error path and only invoke it after all startup steps succeed),
or refactor to avoid global state by introducing a registry tied to the server
instance (e.g., replace package-global sealing with an instance method on a
registry object created/owned by Start and initialized via
audit.ApplyTypeRegistrations(startConfig.auditTypeRegistrations)), ensuring
audit.ApplyTypeRegistrations can still be called on retries before any seal is
applied.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 2, 2026

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants