Skip to content

Added logger#33

Merged
wawrzyniec-madej merged 1 commit intomainfrom
chore/implement-ditto-healthcheck-logger
Apr 8, 2026
Merged

Added logger#33
wawrzyniec-madej merged 1 commit intomainfrom
chore/implement-ditto-healthcheck-logger

Conversation

@wawrzyniec-madej
Copy link
Copy Markdown

@wawrzyniec-madej wawrzyniec-madej commented Apr 8, 2026

Description

Brief description of your changes

AI Assistance Tracking

We're running a metric to understand where AI assists our engineering work. Please select exactly one of the options below:

Mark "Yes" if AI helped in any part of this work, for example: generating code, refactoring, debugging support, explaining something, reviewing an idea, or suggesting an approach.

  • Yes, AI assisted with this PR
  • No, AI did not assist with this PR

@wawrzyniec-madej wawrzyniec-madej requested review from a team and kibertoad as code owners April 8, 2026 10:26
@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Apr 8, 2026

📝 Walkthrough

Summary by CodeRabbit

  • Refactor
    • Improved internal dependency injection configuration and enhanced logging integration for better operational visibility during error scenarios.

Walkthrough

The changes establish a logger as a mandatory dependency in the Dependency Injection configuration and integrate it into the Ditto API client. The ExternalDependencies interface now requires a logger: FastifyBaseLogger (previously optional), and the registerDependencies function signature is updated to accept this explicitly. The logger is registered in the DI container and exposed through the Dependencies interface. The APIDitto client is updated to accept and utilize the injected logger for structured error logging when health checks encounter unexpected failures.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

🚥 Pre-merge checks | ❌ 3

❌ Failed checks (2 warnings, 1 inconclusive)

Check name Status Explanation Resolution
Description check ⚠️ Warning The description section contains only a placeholder with no actual content explaining the changes, failing to provide meaningful context for reviewers. Fill in the 'Description' section with a clear explanation of why the logger was added, what components were modified, and how the DI container changes affect the application.
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
Title check ❓ Inconclusive The title 'Added logger' is vague and generic, failing to convey the specific nature or scope of the changes made to the codebase. Use a more descriptive title that explains what was done with the logger, such as 'Make logger required in dependency injection and add logging to Ditto health checks'.

✏️ 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 chore/implement-ditto-healthcheck-logger

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

🧹 Nitpick comments (1)
src/integrations/ditto/client/APIDitto.ts (1)

182-193: Log structured error fields instead of inspect(response).

Using util.inspect() converts the response to an unqueryable string, making logs harder to parse and search. Since your logger is FastifyBaseLogger (Pino-based), it automatically serializes objects—just pass the structure directly. Replace the stringified inspect() with bounded fields such as statusCode:

Suggested refactor
this.logger.error(
  {
    healthcheck: 'ditto_api',
    requestMethod: 'GET',
    requestPath: healthCheckPath,
    statusCode: response.error?.statusCode,
  },
  'Ditto API healthcheck failed: unexpected response from Ditto',
)

This keeps logs compact and queryable without losing relevant diagnostics.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/integrations/ditto/client/APIDitto.ts` around lines 182 - 193, The
current APIDitto.ts healthcheck log uses util.inspect(response) which turns the
response into an unqueryable string; update the this.logger.error call in the
Ditto API healthcheck (look for logger.error, healthCheckPath and the response
variable) to pass structured fields instead (e.g. statusCode:
response.error?.statusCode || response.statusCode, maybe response.body or
response.headers as needed) and include a short message as the second argument
(e.g. "Ditto API healthcheck failed: unexpected response from Ditto") so logs
remain compact and queryable.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@src/integrations/ditto/client/APIDitto.ts`:
- Around line 182-193: The current APIDitto.ts healthcheck log uses
util.inspect(response) which turns the response into an unqueryable string;
update the this.logger.error call in the Ditto API healthcheck (look for
logger.error, healthCheckPath and the response variable) to pass structured
fields instead (e.g. statusCode: response.error?.statusCode ||
response.statusCode, maybe response.body or response.headers as needed) and
include a short message as the second argument (e.g. "Ditto API healthcheck
failed: unexpected response from Ditto") so logs remain compact and queryable.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository: lokalise/coderabbit/.coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: e3964288-361b-4cc8-8117-75c145c31aa1

📥 Commits

Reviewing files that changed from the base of the PR and between 3457ed1 and 4a69e2d.

📒 Files selected for processing (2)
  • src/infrastructure/diConfig.ts
  • src/integrations/ditto/client/APIDitto.ts

@wawrzyniec-madej wawrzyniec-madej merged commit 3ff1db9 into main Apr 8, 2026
4 of 5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants