Conversation
WalkthroughThe changes introduced in this pull request include the addition of a new logging dependency in the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
|
/build-deploy-preview skip-tests=true |
github-actions
bot
added
the
skip-changelog
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/11718111309. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
app/server/appsmith-server/src/main/resources/logback-spring.xml (2)
3-6: Consider adding environment variable documentation.The configuration uses environment variables
HOSTNAMEandAPPSMITH_DEPLOYMENT_NAME. Consider adding a comment block documenting these environment variables and their expected values for better maintainability.
12-14: Consider enhancing the dev/test logging pattern.The current pattern misses log level and logger name, which are useful for debugging. Consider adding these fields:
- [%d{ISO8601, UTC}] [%t] requestId=%X{X-Appsmith-Request-Id} userEmail=%X{userEmail} traceId=%X{traceId} spanId=%X{spanId} - %m%n + [%d{ISO8601, UTC}] [%level] [%logger] [%t] requestId=%X{X-Appsmith-Request-Id} userEmail=%X{userEmail} traceId=%X{traceId} spanId=%X{spanId} - %m%n
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
app/server/appsmith-interfaces/pom.xml(1 hunks)app/server/appsmith-server/src/main/resources/application.properties(0 hunks)app/server/appsmith-server/src/main/resources/logback-spring.xml(1 hunks)
💤 Files with no reviewable changes (1)
- app/server/appsmith-server/src/main/resources/application.properties
🔇 Additional comments (1)
app/server/appsmith-interfaces/pom.xml (1)
21-26: Consider using the latest stable version 7.4
The dependency addition aligns with the JSON logging objective. However, version 8.0 might be pre-release. Consider using the latest stable version 7.4 unless there's a specific feature requirement.
| <encoder class="net.logstash.logback.encoder.LoggingEventCompositeJsonEncoder"> | ||
| <providers> | ||
| <!-- Add basic information --> | ||
| <timestamp> | ||
| <fieldName>timestamp</fieldName> | ||
| </timestamp> | ||
|
|
||
| <contextName/> | ||
|
|
||
| <!-- Custom fields for deployment and service instance information --> | ||
| <customFields> | ||
| { | ||
| "deploymentName": "${DEPLOYMENT_NAME}", | ||
| "serviceInstanceId": "${SERVICE_INSTANCE_ID}" | ||
| } | ||
| </customFields> | ||
|
|
||
| <!-- Use PatternJsonProvider for structured fields --> | ||
| <pattern> | ||
| <pattern> | ||
| { | ||
| "level": "%level", | ||
| "logger": "%logger", | ||
| "thread": "%thread", | ||
| "message": "%message", | ||
| "exception": "%exception", | ||
| "requestId": "%X{X-Appsmith-Request-Id}", | ||
| "userEmail": "%X{userEmail}", | ||
| "traceId": "%X{traceId}", | ||
| "spanId": "%X{spanId}" | ||
| } | ||
| </pattern> | ||
| </pattern> | ||
| </providers> | ||
| </encoder> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider optimizing JSON logging configuration.
The JSON encoder configuration could benefit from stack trace compaction and line separation:
<encoder class="net.logstash.logback.encoder.LoggingEventCompositeJsonEncoder">
+ <jsonGeneratorDecorator class="net.logstash.logback.decorate.CompositeJsonGeneratorDecorator">
+ <decorator class="net.logstash.logback.decorate.PrettyPrintingDecorator"/>
+ </jsonGeneratorDecorator>
<providers>
+ <stackTrace>
+ <throwableConverter class="net.logstash.logback.stacktrace.ShortenedThrowableConverter">
+ <maxDepthPerThrowable>30</maxDepthPerThrowable>
+ <maxLength>2048</maxLength>
+ <shortenedClassNameLength>20</shortenedClassNameLength>
+ </throwableConverter>
+ </stackTrace>
This will prevent stack traces from becoming too verbose in production logs.
Committable suggestion skipped: line range outside the PR's diff.
|
Deploy-Preview-URL: https://ce-37272.dp.appsmith.com |
sharat87
left a comment
sharat87
left a comment
There was a problem hiding this comment.
Can you put in some example log lines in the PR description please? I'm especially interested to see how stack traces show up and if they become unreadable when reading logs as a text file.
|
This PR has not seen activitiy for a while. It will be closed in 7 days unless further activity is detected. |
|
This PR has been closed because of inactivity. |
2 participants
Description
Added configurations to enable json logging for server that allows use to have structured labels for all logs coming out of server
Automation
/ok-to-test tags=""
🔍 Cypress test results
Caution
If you modify the content in this section, you are likely to disrupt the CI result for your PR.
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit