Skip to content

Feature/json logging #8

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Feature/json logging #8

wants to merge 2 commits into from

Conversation

sigrdrifa
Copy link
Contributor

This pull request introduces significant enhancements to logging functionality, including the addition of JSON-based logging for production environments, structured logging capabilities, and environment-specific log configurations. It also updates the application to use these new logging features throughout its lifecycle.

Logging Enhancements:

  • Introduced a new LogFormat type to support both text-based and JSON-based logging formats, with JSON logging designed for production environments. (src/Logger.hs, src/Logger.hsR62-R142)
  • Added helper functions (logInfoJSON, logErrorJSON, logDebugJSON) for structured JSON logging, enabling the inclusion of detailed metadata in log messages. (src/Logger.hs, src/Logger.hsR167-R184)
  • Created a createLogEnv function to configure the logging environment based on the selected format. (src/Logger.hs, src/Logger.hsR62-R142)

Middleware and Application Logging:

  • Implemented a new setJSONLogger middleware that logs HTTP requests and responses in JSON format, including metadata such as method, path, status code, and duration. (src/Config.hs, src/Config.hsL89-R138)
  • Updated the initialize function to include JSON-based logging during application startup, configuration loading, and database migrations. (src/Init.hs, [1] [2]

Dependency Updates:

  • Added aeson-pretty and http-types as new dependencies to support JSON logging and HTTP metadata handling. (hastl.cabal, hastl.cabalR43-R45)

@sigrdrifa sigrdrifa requested a review from Copilot June 10, 2025 16:05
Copy link
Contributor

@Copilot 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

This PR enhances the application’s logging by introducing a JSON logging format alongside the existing text format, adding structured logging helpers, and configuring environment-specific log behavior.

  • Define LogFormat, createLogEnv, and a JSON scribe in Logger.hs
  • Implement JSON-based HTTP request/response middleware in Config.hs and integrate JSON logging into initialization in Init.hs
  • Update hastl.cabal with aeson-pretty and http-types dependencies

Reviewed Changes

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

File Description
src/Logger.hs Added LogFormat, JSON scribe, itemToJSON, and helpers
src/Init.hs Use JSON logging during startup/migrations; env-based setup
src/Config.hs Add setJSONLogger middleware for structured HTTP logging
hastl.cabal Added aeson-pretty and http-types dependencies
Comments suppressed due to low confidence (1)

src/Logger.hs:178

  • New JSON logging helpers (logInfoJSON, logErrorJSON, logDebugJSON) and the middleware lack accompanying unit or integration tests. Adding tests will ensure correct behavior and ease future refactoring.
logInfoJSON msg payload = logJSON mempty InfoS msg payload

-- Production uses JSON format, Development and Test use text format
createLogEnvForEnvironment :: Environment -> IO LogEnv
createLogEnvForEnvironment Production = createLogEnv JSONFormat
createLogEnvForEnvironment Development = createLogEnv JSONFormat
Copy link
Preview

Copilot AI Jun 10, 2025

Choose a reason for hiding this comment

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

The comment above createLogEnvForEnvironment states Development should use text format, but the code sets Development to JSONFormat. Align the implementation with the comment or update the comment to match intended behavior.

Suggested change
createLogEnvForEnvironment Development = createLogEnv JSONFormat
createLogEnvForEnvironment Development = createLogEnv TextFormat

Copilot uses AI. Check for mistakes.

-- | Environment-aware logger that uses JSON for production
setJSONLogger :: Environment -> LogEnv -> Middleware
setJSONLogger Test _ = id
setJSONLogger Development env = katipLogger env
Copy link
Preview

Copilot AI Jun 10, 2025

Choose a reason for hiding this comment

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

This applies JSON logging in Development too, conflicting with the comment that JSON is only for production. Consider restricting JSON middleware to Production or update the comment accordingly.

Suggested change
setJSONLogger Development env = katipLogger env
setJSONLogger Development _ = id

Copilot uses AI. Check for mistakes.

Comment on lines +94 to +97
lockingAction <- FastLogger.newStdoutLoggerSet FastLogger.defaultBufSize >>= \loggerSet ->
return $ \logStrData -> do
FastLogger.pushLogStr loggerSet logStrData
FastLogger.flushLogStr loggerSet
Copy link
Preview

Copilot AI Jun 10, 2025

Choose a reason for hiding this comment

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

Flushing the logger set on every log event can become a performance bottleneck under high throughput. Consider using asynchronous or batched flushing.

Suggested change
lockingAction <- FastLogger.newStdoutLoggerSet FastLogger.defaultBufSize >>= \loggerSet ->
return $ \logStrData -> do
FastLogger.pushLogStr loggerSet logStrData
FastLogger.flushLogStr loggerSet
loggerSet <- FastLogger.newStdoutLoggerSet FastLogger.defaultBufSize
let lockingAction logStrData = FastLogger.pushLogStr loggerSet logStrData

Copilot uses AI. Check for mistakes.

[ "timestamp" .= timestamp
, "level" .= severityToText (_itemSeverity item)
, "namespace" .= nsText
, "message" .= (T.strip $ T.replace "LogStr {unLogStr = \"" "" $ T.replace "\"}" "" msgText)
Copy link
Preview

Copilot AI Jun 10, 2025

Choose a reason for hiding this comment

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

[nitpick] The manual string replacements to extract the log message are brittle and hard to read. Consider using a proper renderer or a dedicated function to convert LogStr to Text safely.

Suggested change
, "message" .= (T.strip $ T.replace "LogStr {unLogStr = \"" "" $ T.replace "\"}" "" msgText)
, "message" .= (T.strip $ logStrToText $ _itemMessage item)

Copilot uses AI. Check for mistakes.

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.

1 participant