Skip to content

Reduce credential-resolution error logging noise (#4814)#4815

Open
stuartc wants to merge 2 commits into
mainfrom
4814-credential-error-log-levels
Open

Reduce credential-resolution error logging noise (#4814)#4815
stuartc wants to merge 2 commits into
mainfrom
4814-credential-error-log-levels

Conversation

@stuartc
Copy link
Copy Markdown
Member

@stuartc stuartc commented May 29, 2026

Description

This PR changes how credential-resolution failures are logged so they stop
flooding Sentry.

These failures were logged with Logger.error from two places (the Resolver
and RunChannel), and three of the five conditions were logged twice. Since
Sentry's LoggerHandler captures :error logs, each one became a Sentry event
— even though most are user-actionable (re-authorise a credential, fix a
project's environment) or transient (provider 429/503), with nothing for us to
fix in code.

Logging is now centralised in Lightning.Credentials.Resolver (one site per
condition) at appropriate levels:

Condition Level Reaches Sentry?
project_not_found error yes
environment_not_configured warning no
environment_mismatch warning no
temporary_failure info no
reauthorization_required info no

Lines stay visible for self-hosted/logs-only operators; they just no longer
page anyone. Also fixes the "OAuth" casing in the two user-facing channel
replies, and adds a short logging guideline.

Closes #4814

Validation steps

  1. mix test test/lightning/credentials/resolver_test.exs — asserts each
    condition logs at its expected level.
  2. Trigger a credential-resolution failure (e.g. an expired OAuth token) and
    confirm it no longer appears in Sentry, while the line is still present in
    the application logs at info/warning.

Additional notes for the reviewer

  1. The metadata/tags_from_metadata cleanup mentioned in the issue is
    deliberately out of scope and tracked separately.

AI Usage

  • I have used Claude Code
  • I have used another model
  • I have not used AI

You can read more details in our
Responsible AI Policy

Pre-submission checklist

  • I have performed an AI review of my code (we recommend using /review
    with Claude Code)
  • I have implemented and tested all related authorization policies.
    (e.g., :owner, :admin, :editor, :viewer) — N/A, logging-only change
  • I have updated the changelog.
  • I have ticked a box in "AI usage" in this PR

stuartc added 2 commits May 29, 2026 17:31
Credential-resolution failures were logged with Logger.error from two
sites (Resolver and RunChannel), and three of five conditions were
logged twice. Because Sentry's LoggerHandler captures :error-level logs,
each one became a Sentry event — mostly for user-actionable or transient
conditions nobody can act on from an ops seat.

Centralise logging in Lightning.Credentials.Resolver (the single
run-credential-resolution layer) via a log_resolution_error/2 helper,
and remove the duplicate Logger.error calls from RunChannel. Per
condition:

  - project_not_found        -> error   (genuine invariant violation)
  - environment_not_configured -> warning (user-actionable config)
  - environment_mismatch     -> warning (user-actionable config)
  - temporary_failure        -> info    (transient provider 429/503)
  - reauthorization_required -> info    (user/IdP state, already audited)

Only project_not_found now reaches Sentry. Also fix the user-facing
"OAuth" casing in the two channel error replies, and add a logging
guideline documenting the level policy and single-log-site rule.
@github-project-automation github-project-automation Bot moved this to New Issues in Core May 29, 2026
@stuartc stuartc requested a review from midigofrank May 29, 2026 15:36
@github-actions
Copy link
Copy Markdown

The PR (#4814) only changes credential-resolution error log levels, removes a duplicate log site in run_channel.ex, fixes an "Oauth" → "OAuth" casing string, and adds tests for the new log levels. No queries, authorization gates, or persistence paths are touched.

Security Review ✅

  • S0 (project scoping): N/A — only log-level routing changed in lib/lightning/credentials/resolver.ex and a Logger.error was removed from lib/lightning_web/channels/run_channel.ex:486-560; the existing socket.assigns.project_id lookup at run_channel.ex:515-516 is unchanged.
  • S1 (authorization): N/A — no new controller actions, handle_event clauses, or web-layer entrypoints introduced.
  • S2 (audit trail): N/A — no Repo.insert/update/delete against config resources; the change is logging-only plus a gettext string fix.

@codecov
Copy link
Copy Markdown

codecov Bot commented May 29, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.3%. Comparing base (6331486) to head (5bdf6be).

Additional details and impacted files
@@           Coverage Diff           @@
##            main   #4815     +/-   ##
=======================================
+ Coverage   90.3%   90.3%   +0.1%     
=======================================
  Files        442     442             
  Lines      22545   22547      +2     
=======================================
+ Hits       20350   20365     +15     
+ Misses      2195    2182     -13     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

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

Labels

None yet

Projects

Status: New Issues

Development

Successfully merging this pull request may close these issues.

Credential-resolution failures are logged as errors (and reported to Sentry) when most aren't faults

1 participant