fix: disable metric formatters by andrestejerina97 · Pull Request #542 · OpenStackweb/summit-api

andrestejerina97 · 2026-05-12T03:28:34Z

ref: https://app.clickup.com/t/86b9pvafe

Summary by CodeRabbit

New Features
- Per-entity audit disabling via configuration.
Improvements
- Audit pipeline now checks per-entity disable flags earlier and skips processing when disabled.
- Public API extended so callers can query whether auditing is disabled for a given subject.
Tests
- Added tests validating per-entity enable/disable behavior and that audit processing returns early when disabled.

coderabbitai · 2026-05-12T03:28:41Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 9df1e8f5-5789-494f-acad-50ce0a1422ee

📥 Commits

Reviewing files that changed from the base of the PR and between c1774e9 and f827a8a.

📒 Files selected for processing (1)

app/Audit/AuditLogFormatterFactory.php

💤 Files with no reviewable changes (1)

app/Audit/AuditLogFormatterFactory.php

📝 Walkthrough

Walkthrough

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.

Changes

Entity-Level Audit Disablement

Layer / File(s)	Summary
Factory subject resolution & disable checks `app/Audit/AuditLogFormatterFactory.php`	Centralizes subject class resolution (`getSubjectClass()`), adds `private isAuditDisabledForSubject()` and public `isAuditDisabled()` methods, and short-circuits `make()` when auditing is disabled for the subject.
OTLP strategy early-exit `app/Audit/AuditLogOtlpStrategy.php`	`AuditLogOtlpStrategy::audit()` now returns early if `formatterFactory->isAuditDisabled($subject)` is true.
Factory interface update `app/Audit/IAuditLogFormatterFactory.php`	Adds `public function isAuditDisabled(mixed $subject): bool;` to the factory interface.
Entity-level audit configuration `config/audit_log.php`	Disable auditing for `\models\summit\SummitEventAttendanceMetric::class` and `\models\summit\SummitMetric::class` by setting their `enabled` flags to `false`.
Tests: factory and strategy `tests/OpenTelemetry/AuditLogFormatterFactoryTest.php`, `tests/OpenTelemetry/AuditLogOtlpStrategyDisabledAuditTest.php`	Add tests (including reflection-driven checks, config injection helper, and test doubles) validating private/public disable checks, that `make()` returns `null` when disabled, and that `AuditLogOtlpStrategy::audit()` returns early and does not enqueue jobs when audits are disabled.

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

OpenStackweb/summit-api#498: Overlaps with changes to AuditLogFormatterFactory::make() and formatter selection logic.

Suggested reviewers

martinquiroga-exo
caseylocker
smarcet

Poem

🐰 I sniffed the config, soft and small,
Quiet metrics nap, no audit call.
The factory peeks, then lets them rest,
Silent logs where noise would have guessed.
—A cheerful Summit Rabbit 🥕

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 5.88% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title "fix: disable metric formatters" aligns with the main changes: disabling audit logging for two metric entity types and implementing the infrastructure to support per-entity audit disabling.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ 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 fix/disable-metric-formatters

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 · 2026-05-12T03:30:35Z

📘 OpenAPI / Swagger preview

➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-542/

This page is automatically updated on each push to this PR.

caseylocker

LGTM. Nothing blocking. Follows the ticket direction.

Copilot

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 SummitEventAttendanceMetric and SummitMetric via config/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;
+        }


github-actions · 2026-05-19T12:44:32Z

📘 OpenAPI / Swagger preview

➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-542/

This page is automatically updated on each push to this PR.

github-actions · 2026-05-19T12:46:37Z

📘 OpenAPI / Swagger preview

➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-542/

This page is automatically updated on each push to this PR.

github-actions · 2026-05-19T17:55:43Z

📘 OpenAPI / Swagger preview

➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-542/

This page is automatically updated on each push to this PR.

smarcet · 2026-05-27T15:40:35Z

+        if (class_exists(ClassUtils::class)) {
+            return ClassUtils::getClass($subject);
+        }


@andrestejerina97
Dead code : ClassUtils isn't available in this project.

This project runs doctrine/orm 3.3.3 with no doctrine/common package, so Doctrine\Common\Util\ClassUtils doesn't exist anywhere in vendor/. class_exists(ClassUtils::class) is therefore always false, this branch never executes, and getSubjectClass() always falls through to get_class() on line 232. The use Doctrine\Common\Util\ClassUtils; import (line 25) is unused at runtime.

This is harmless for the metric-disable goal — the audit listener passes real entity instances from the UnitOfWork (not lazy proxies), and get_class() already matched these entities before this PR. But the code reads as if Doctrine proxies are handled when they aren't, which is misleading for the next reader.

Suggest one of:

Remove the ClassUtils import and this branch, keeping just get_class($subject); or

If proxy-safe resolution is actually intended, do it the ORM 3 way, e.g. $em->getClassMetadata(get_class($subject))->getName() or a Doctrine\Persistence\Proxy instanceof check.

github-actions · 2026-05-27T19:37:53Z

📘 OpenAPI / Swagger preview

➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-542/

This page is automatically updated on each push to this PR.

fix: disable metric formatters

645f3f3

andrestejerina97 marked this pull request as ready for review May 12, 2026 11:38

caseylocker requested review from caseylocker and smarcet May 12, 2026 14:52

caseylocker approved these changes May 12, 2026

View reviewed changes

smarcet requested a review from Copilot May 13, 2026 14:27

Copilot started reviewing on behalf of smarcet May 13, 2026 14:28 View session

Copilot AI reviewed May 13, 2026

View reviewed changes

andrestejerina97 force-pushed the fix/disable-metric-formatters branch from 7ff8e9c to 645f3f3 Compare May 19, 2026 12:44

fix: remove the entry when the formatter is disabled

c1774e9

smarcet reviewed May 27, 2026

View reviewed changes

fix: remove dead code

f827a8a

andrestejerina97 requested a review from smarcet May 27, 2026 19:50

Conversation

andrestejerina97 commented May 12, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented May 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Sequence Diagram(s)

Estimated code review effort

Possibly related PRs

Suggested reviewers

Poem

❌ Failed checks (1 warning)

Uh oh!

github-actions Bot commented May 12, 2026

Uh oh!

caseylocker left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

github-actions Bot commented May 19, 2026

Uh oh!

github-actions Bot commented May 19, 2026

Uh oh!

github-actions Bot commented May 19, 2026

Uh oh!

smarcet May 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented May 27, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

andrestejerina97 commented May 12, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented May 12, 2026 •

edited

Loading

smarcet May 27, 2026 •

edited

Loading