Skip to content

feat: skip log collector on hot path when admin API is disabled#118

Merged
skyoo2003 merged 1 commit into
mainfrom
perf/112-conditional-log-collector
Jul 24, 2026
Merged

feat: skip log collector on hot path when admin API is disabled#118
skyoo2003 merged 1 commit into
mainfrom
perf/112-conditional-log-collector

Conversation

@skyoo2003

Copy link
Copy Markdown
Owner

Summary

Skip per-request log collection when the admin API is disabled (the default), removing needless bookkeeping from the AWS request hot path.

Related Issue

Fixes #112

Changes

  • cmd/devcloud/main.go: build the admin.LogCollector only inside the if cfg.Admin.Enabled block; pass nil to gateway.New when admin is off.
  • internal/gateway/gateway.go: install the logging middleware only when logCollector != nil. With a nil collector the service router is used directly, so the mutex lock + ring-buffer write, newStatusRecorder, time.Now, and detectService all drop off the hot path. New signature is unchanged (now nil-tolerant).
  • internal/gateway/gateway_test.go: add TestGateway_NilLogCollector covering the disabled/nil path (request succeeds, no panic, no Add).
  • Changie fragment for the change.

Test Plan

  • go build ./...
  • go test ./internal/gateway/ ./internal/admin/ ./cmd/... — all pass. Admin-enabled behavior (recent-logs endpoint) stays covered by internal/admin/api_test.go; collector semantics by logger_test.go.
  • golangci-lint run ./internal/gateway/... ./cmd/... — 0 issues.

Checklist

  • Self-reviewed the code
  • Added/updated tests
  • Lint/format passes (golangci-lint run)
  • Updated documentation (if applicable) — N/A (behavior identical, no user-facing config change)
  • Added a Changie changelog fragment for user-facing changes

Build the admin.LogCollector only when admin is enabled and pass nil to
gateway.New otherwise. The logging middleware is now installed only when
a collector exists, so a disabled admin API no longer pays a per-request
mutex lock + ring-buffer write that no consumer could read.

Fixes #112
@sourcery-ai

sourcery-ai Bot commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

Reviewer's Guide

This PR makes the gateway tolerant of a nil admin log collector and wires devcloud so that the log collector (and associated logging middleware) is only created and installed when the admin API is enabled, keeping logging-related mutex and ring-buffer operations off the AWS request hot path while adding a regression test and changelog entry.

File-Level Changes

Change Details Files
Make gateway logging middleware conditional on presence of a log collector, keeping the hot path lean when logging is disabled.
  • Wrap the service router in a logging middleware only when the provided log collector is non-nil, otherwise use the router directly as the service handler.
  • Preserve the Gateway.New function signature while making it explicitly nil-tolerant for the log collector parameter.
  • Continue to wrap the chosen service handler with existing middlewares (error recovery, body limit, CORS, logging, router) via ChainMiddleware.
internal/gateway/gateway.go
Add coverage for the nil-log-collector code path to ensure safe operation when admin logging is disabled.
  • Introduce TestGateway_NilLogCollector to construct a gateway with a nil log collector and a stubbed plugin registry.
  • Verify that an HTTP request succeeds with status 200 without panicking or requiring the logging middleware to run.
internal/gateway/gateway_test.go
Only construct the admin log collector when the admin API is enabled, otherwise pass nil to the gateway to avoid unnecessary per-request logging overhead.
  • Replace unconditional creation of the admin log collector with a lazily initialized variable used only when admin.enabled is true.
  • Pass a possibly nil log collector into gateway.New so that when admin is disabled the gateway runs without the logging middleware while still exposing a 404 admin handler.
cmd/devcloud/main.go
Document the behavior change in the unreleased changelog.
  • Add a Changie fragment describing that request log collection is skipped on the AWS request hot path when admin is disabled and referencing the related issue.
changes/unreleased/Changed-20260725-000001.yaml

Assessment against linked issues

Issue Objective Addressed Explanation
#112 Make the admin log collector optional so that when admin.enabled is false, no per-request logCollector.Add work (mutex lock + ring-buffer insert and related bookkeeping) is done on the AWS request hot path.
#112 Preserve existing behavior when admin.enabled is true, including the recent-logs endpoint and its tests, so that admin-enabled logging semantics remain unchanged.

Possibly linked issues


Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

@github-actions github-actions Bot added the tests Test code and test infrastructure label Jul 24, 2026
@skyoo2003 skyoo2003 self-assigned this Jul 24, 2026
@skyoo2003 skyoo2003 changed the title perf: skip log collector on hot path when admin API is disabled feat: skip log collector on hot path when admin API is disabled Jul 24, 2026

@sourcery-ai sourcery-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hey - I've reviewed your changes and they look great!


Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

@skyoo2003
skyoo2003 merged commit 5b25062 into main Jul 24, 2026
8 checks passed
@skyoo2003
skyoo2003 deleted the perf/112-conditional-log-collector branch July 24, 2026 16:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

tests Test code and test infrastructure

Projects

None yet

Development

Successfully merging this pull request may close these issues.

perf: admin log collector runs on the hot path even when the admin API is disabled

1 participant