fix: disable metric formatters#542
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThis PR adds per-entity audit-disable checks to AuditLogFormatterFactory (centralizing subject class resolution) and makes AuditLogOtlpStrategy exit early when an entity is configured disabled. Two summit metric entities are disabled in config and tests cover the factory and strategy behavior. ChangesEntity-Level Audit Disablement
Sequence Diagram(s)sequenceDiagram
participant Client
participant AuditLogOtlpStrategy
participant AuditLogFormatterFactory
participant Config
participant Queue
Client->>AuditLogOtlpStrategy: audit(ctx, subject, event)
AuditLogOtlpStrategy->>AuditLogFormatterFactory: isAuditDisabled(subject)
AuditLogFormatterFactory->>Config: read audit_log.entities[getSubjectClass(subject)]['enabled']
alt enabled === false
AuditLogFormatterFactory-->>AuditLogOtlpStrategy: true
AuditLogOtlpStrategy-->>Client: return (no enqueue)
else
AuditLogFormatterFactory-->>AuditLogOtlpStrategy: false
AuditLogOtlpStrategy->>AuditLogFormatterFactory: make(ctx, subject, event)
AuditLogOtlpStrategy->>Queue: dispatch EmitAuditLogJob
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-542/ This page is automatically updated on each push to this PR. |
caseylocker
left a comment
There was a problem hiding this comment.
LGTM. Nothing blocking. Follows the ticket direction.
There was a problem hiding this comment.
Pull request overview
This PR adds a configuration-driven way to disable audit logging for specific entity types, and uses it to disable audit logging for metric entities (notably SummitEventAttendanceMetric and SummitMetric) in the OpenTelemetry audit logging pipeline.
Changes:
- Add an entity “enabled/disabled” guard to
AuditLogFormatterFactory::make()to skip formatter creation for disabled entities. - Disable audit logging for
SummitEventAttendanceMetricandSummitMetricviaconfig/audit_log.php. - Extend unit tests to cover per-entity audit-disable behavior and factory output when disabled.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| app/Audit/AuditLogFormatterFactory.php | Adds a config-based early return to skip audit formatter creation when an entity is disabled. |
| config/audit_log.php | Sets enabled => false for metric entities to disable their audit logging. |
| tests/OpenTelemetry/AuditLogFormatterFactoryTest.php | Adds test coverage for the new “audit disabled” logic and factory behavior. |
Comments suppressed due to low confidence (1)
tests/OpenTelemetry/AuditLogFormatterFactoryTest.php:24
- The class-level docblock still says these tests are only for
matchesStrategy()null-guard behavior, but this file now also tests audit-disable behavior (isAuditDisabledForSubject/make). Updating the docblock would keep the test intent accurate.
use App\Audit\Interfaces\IAuditStrategy;
use PHPUnit\Framework\TestCase;
/**
* Class AuditLogFormatterFactoryTest
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| $entities = $this->config['entities'] ?? []; | ||
| $entity_config = $entities[get_class($subject)] ?? null; | ||
|
|
||
| return is_array($entity_config) | ||
| && array_key_exists('enabled', $entity_config) | ||
| && $entity_config['enabled'] === false; |
| if ($this->isAuditDisabledForSubject($subject)) { | ||
| return null; | ||
| } |
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-542/ This page is automatically updated on each push to this PR. |
7ff8e9c to
645f3f3
Compare
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-542/ This page is automatically updated on each push to this PR. |
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-542/ This page is automatically updated on each push to this PR. |
ref: https://app.clickup.com/t/86b9pvafe
Summary by CodeRabbit
New Features
Improvements
Tests