Skip to content

fix: drop creating a child logger per req#1050

Merged
ferhatelmas merged 1 commit intomasterfrom
ferhat/no-child-logger-per-request
Apr 24, 2026
Merged

fix: drop creating a child logger per req#1050
ferhatelmas merged 1 commit intomasterfrom
ferhat/no-child-logger-per-request

Conversation

@ferhatelmas
Copy link
Copy Markdown
Member

What kind of change does this PR introduce?

refactor

What is the current behavior?

Creating a child logger per request to inject tenant, reqId, sbReqId and version.

What is the new behavior?

Carry them through request to the log calls not to copy logger.
Version is static and set with region in base logger creation.

Additional context

project field could be dropped separately i.e. only moved to top level by checking context.type:request

Copilot AI review requested due to automatic review settings April 24, 2026 12:12
@ferhatelmas ferhatelmas requested a review from a team as a code owner April 24, 2026 12:12
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Refactors request logging to avoid creating a per-request child logger, instead threading tenantId, reqId, sbReqId, and version context through structured log payloads (with version/region bound at base logger creation).

Changes:

  • Bind region + appVersion on the shared base logger; introduce a reusable RequestLogContext type.
  • Remove per-request request.log.child(...) from tenant-id plugins; update log call sites to pass request context fields explicitly.
  • Add/adjust tests to assert context threading and to prevent per-request child logger creation.

Reviewed changes

Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
src/start/server.ts Uses the shared logger for admin startup failure logging.
src/internal/monitoring/logger.ts Binds appVersion at logger creation; introduces RequestLogContext and threads it through log schema types.
src/http/routes/tus/lifecycle.ts Threads tenant/request context into TUS lifecycle logSchema calls.
src/http/routes/tus/lifecycle.test.ts Updates expectations to include threaded tenantId/project/reqId fields.
src/http/routes/tus/index.ts Persists reqId onto the raw upload context for later log calls.
src/http/routes/admin/objects.ts Threads tenant/request context into orphan-object admin error logs.
src/http/routes/admin/jwks.ts Threads tenant/request context into generator error logs.
src/http/plugins/tracing.ts Threads tenant/request context into tracing-mode failure logs.
src/http/plugins/tenant-id.ts Removes per-request child logger creation from tenant-id plugins.
src/http/plugins/tenant-id.test.ts Adds regression tests to ensure tenant-id plugins don’t call request.log.child().
src/http/plugins/log-request.ts Threads tenant/request context into request logging and metadata failure logs.
src/http/plugins/log-request.test.ts Adds coverage to ensure tenant context is included in request log output.
src/http/plugins/db.ts Threads tenant/request context into db-dispose failure logs across lifecycle hooks.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/internal/monitoring/logger.ts
Comment thread src/http/plugins/tenant-id.ts
@coveralls
Copy link
Copy Markdown

coveralls commented Apr 24, 2026

Coverage Report for CI Build 24894671802

Coverage decreased (-0.01%) to 71.454%

Details

  • Coverage decreased (-0.01%) from the base build.
  • Patch coverage: 4 of 4 lines across 2 files are fully covered (100%).
  • 2 coverage regressions across 1 file.

Uncovered Changes

No uncovered changes found.

Coverage Regressions

2 previously-covered lines in 1 file lost coverage.

File Lines Losing Coverage Coverage
src/http/routes/admin/objects.ts 2 28.85%

Coverage Stats

Coverage Status
Relevant Lines: 9754
Covered Lines: 7370
Line Coverage: 75.56%
Relevant Branches: 5453
Covered Branches: 3496
Branch Coverage: 64.11%
Branches in Coverage %: Yes
Coverage Strength: 377.15 hits per line

💛 - Coveralls

@ferhatelmas ferhatelmas force-pushed the ferhat/no-child-logger-per-request branch 3 times, most recently from d2cc98e to 53e46a4 Compare April 24, 2026 12:25
@ferhatelmas ferhatelmas requested a review from Copilot April 24, 2026 12:25
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 17 out of 17 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (4)

src/storage/events/objects/object-admin-delete.ts:55

  • logSchema.event now emits jobId, but the error log in the catch block still uses the old jodId key. This will fragment event/job correlation in logs (success vs failure paths use different fields). Update the catch logging to use jobId consistently (and ideally route through logSchema.error for schema consistency).
      logSchema.event(logger, `[Admin]: ObjectAdminDelete ${s3Key}`, {
        jobId: job.id,
        type: 'event',
        event: 'ObjectAdminDelete',
        payload: JSON.stringify(job.data),
        objectPath: s3Key,
        resources: [`${job.data.bucketId}/${job.data.name}`],
        tenantId: job.data.tenant.ref,
        project: job.data.tenant.ref,
        reqId: job.data.reqId,
        sbReqId: job.data.sbReqId,
      })

src/storage/events/objects/object-admin-delete-all-before.ts:57

  • logSchema.event was updated to use jobId, but the failure path later in this handler still logs jodId. Please align the failure log field name to jobId as well so dashboards/queries don’t need to handle both spellings.
      logSchema.event(
        logger,
        `[Admin]: ObjectAdminDeleteAllBefore ${bucketId} ${before.toUTCString()}`,
        {
          jobId: job.id,
          type: 'event',
          event: 'ObjectAdminDeleteAllBefore',
          payload: JSON.stringify(job.data),
          objectPath: bucketId,
          tenantId,
          project: tenantId,
          reqId: job.data.reqId,
          sbReqId: job.data.sbReqId,
        }

src/storage/events/objects/backup-object.ts:69

  • In this handler, the success logs use jobId and event: 'BackupObject', but the catch block still logs jodId and sets event: 'ObjectAdminDelete'. That mismatch makes it hard to reliably filter/aggregate BackupObject failures. Update the failure log to use jobId and the same event value as the success path.
      logSchema.event(logger, `[Admin]: BackupObject ${s3Key}`, {
        jobId: job.id,
        type: 'event',
        event: 'BackupObject',
        payload: JSON.stringify(job.data),
        objectPath: s3Key,
        resources: [`${job.data.bucketId}/${job.data.name}`],
        tenantId: job.data.tenant.ref,
        project: job.data.tenant.ref,
        reqId: job.data.reqId,
        sbReqId: job.data.sbReqId,
      })

src/storage/events/lifecycle/webhook.ts:160

  • logSchema.event now emits jobId, but the failure log in the catch block below still uses jodId. Please switch the failure log field name to jobId as well to keep webhook event logs consistent between success/failure paths.
    logSchema.event(logger, `[Lifecycle]: ${job.data.event.type} ${path}`, {
      jobId: job.id,
      type: 'event',
      event: job.data.event.type,
      payload: JSON.stringify(job.data.event.payload),
      objectPath: path,
      resources: ['/' + path],
      tenantId: job.data.tenant.ref,
      project: job.data.tenant.ref,
      reqId: job.data.event.payload.reqId,
      sbReqId: job.data.event.payload.sbReqId,
    })

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@ferhatelmas ferhatelmas force-pushed the ferhat/no-child-logger-per-request branch from 53e46a4 to f5cf761 Compare April 24, 2026 13:06
Signed-off-by: ferhat elmas <elmas.ferhat@gmail.com>
@ferhatelmas ferhatelmas force-pushed the ferhat/no-child-logger-per-request branch from f5cf761 to 6ccf893 Compare April 24, 2026 14:26
@ferhatelmas ferhatelmas merged commit c1c952f into master Apr 24, 2026
9 checks passed
@ferhatelmas ferhatelmas deleted the ferhat/no-child-logger-per-request branch April 24, 2026 14:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants