feat: use log4j2 everywhere in spark consistently

[tchow-zlai]

Summary

Testing this locally:

I see no spark logging churn. Looks like it preserves @nikhil-zlai 's PR's behavior: #96

Checklist

• Added Unit Tests
• Covered by existing CI
• Integration tested
• Documentation update

Summary by CodeRabbit

• New Features

  • Introduced a new logging configuration using Log4j2, enhancing logging capabilities and readability.

• Bug Fixes

  • Removed outdated logging configuration references, streamlining Docker container execution and Spark application setup.

• Chores

  • Updated dependency management to replace Logback with Log4j2 for consistent logging behavior across the project.
  • Enhanced CI/CD workflows to trigger on changes to the build.sbt file, improving responsiveness to updates.

[coderabbitai]

Walkthrough

The changes in this pull request involve modifications to the logging configuration across several files. The build.sbt file has been updated to replace the Logback logging library with Log4j2, introducing new dependencies and removing old ones. The run.sh script has been simplified by removing references to a log configuration file, indicating a shift in how logging is handled during Docker container execution. Additionally, a new log4j2.properties file has been created to define the logging behavior for Log4j2, including log levels and formatting.

Changes

File	Change Summary
build.sbt	- Added new Log4j2 dependency (log4j-slf4j-impl version 2.20.0). - Removed Logback dependency (logback-classic version 1.2.13). - Updated exclusion rules for logging dependencies in the hub project.
docker-init/demo/run.sh	- Removed the LOG_CONFIG_FILE variable and its usage in the Docker run command and spark-submit.
spark/src/main/resources/log4j2.properties	- Introduced a new logging configuration for Log4j2, specifying log levels and appender settings.
.github/workflows/test_scala_no_spark.yaml	- Updated to monitor changes in build.sbt for triggering the workflow.
.github/workflows/test_scala_spark.yaml	- Updated to monitor changes in build.sbt for triggering the workflow.

Possibly related PRs

• feat: enable slf4j logging to work with logback #76: This PR modifies the build.sbt file to include a dependency for logback-classic, which is directly related to the changes in the main PR that involve switching from Logback to Log4j2 for logging.
• Driver Summarizer #62: Modifications related to logging dependencies and configurations, relevant to data handling and logging aspects.
• Summary upload #50: Introduction of the SummaryUploader class aligns with the logging changes, enhancing data handling and logging mechanisms.
• Update feature time series api to return current vs baseline #53: Updates to the TimeSeriesController involve logging improvements, related to the main PR's focus on logging.
• Wire up Play frontend + server in docker setup #24: Changes in Docker setup may involve logging configurations, relevant to the main PR's logging updates.
• Docker init #20: Modifications in Docker initialization relate to the logging changes in the main PR.

Suggested reviewers

• nikhil-zlai
• piyush-zlai

Poem

🐰 In the woods where the loggers play,
A new friend, Log4j2, came to stay.
With colors bright and messages clear,
Our logs now sing for all to hear!
Goodbye to Logback, we wave and cheer,
For a simpler path is finally here! 🌟

Warning

Rate limit exceeded

@tchow-zlai has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 9 minutes and 33 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

📥 Commits

Reviewing files that changed from the base of the PR and between 0f27fd6 and 267151e.

---

📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 0f27fd6 and 267151e.

📒 Files selected for processing (5)

• .github/workflows/test_scala_no_spark.yaml (2 hunks)
• .github/workflows/test_scala_spark.yaml (1 hunks)
• build.sbt (2 hunks)
• docker-init/demo/run.sh (1 hunks)
• spark/src/main/resources/log4j2.properties (1 hunks)

🚧 Files skipped from review as they are similar to previous changes (3)

• build.sbt
• docker-init/demo/run.sh
• spark/src/main/resources/log4j2.properties

🔇 Additional comments (4)

.github/workflows/test_scala_no_spark.yaml (2)

13-13: LGTM! Good addition to trigger paths

Adding build.sbt to the workflow triggers is appropriate since it now contains logging configuration changes that could affect test behavior.

Also applies to: 23-23

---

13-13: Verify corresponding changes in spark workflow

Since this PR aims for consistent logging across the codebase, we should ensure the spark testing workflow is similarly updated.

Also applies to: 23-23

✅ Verification successful

Changes in spark workflow are consistent with no-spark workflow

The verification confirms that build.sbt is already included in the paths section of the spark workflow, maintaining consistency with the no-spark workflow.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify if spark workflow has similar build.sbt trigger
# Expected: Should find build.sbt in paths section of spark workflow

rg -A 5 "paths:" .github/workflows/test_scala_spark.yaml

Length of output: 265

.github/workflows/test_scala_spark.yaml (2)

10-10: LGTM! Good addition of build.sbt to workflow triggers.

Adding build.sbt to the workflow triggers is essential since the PR involves logging library changes. This ensures that any modifications to the logging dependencies will trigger the test suite.

Also applies to: 17-17

---

Line range hint 1-93: Verify logging behavior in Spark tests

Since this PR implements log4j2 consistently across the codebase, we should verify that the Spark tests properly exercise the logging configuration.

Let's check for logging-related test coverage:

Consider adding specific test cases to verify:

1. Log4j2 configuration is properly loaded in Spark context
2. Log levels are respected
3. No unexpected logging churn occurs during Spark operations

✅ Verification successful

Based on the search results, I can now generate the final response:

Spark tests properly exercise logging configuration

The codebase shows comprehensive logging test coverage:

• A central log4j2 configuration exists at spark/src/main/resources/log4j2.properties
• Tests properly initialize logging through SparkSessionBuilder
• Multiple test classes use SLF4J Logger for test assertions and verification:
  • StatsComputeTest
  • DerivationTest
  • LogBootstrapTest
  • FeatureWithLabelJoinTest
  • ChainingFetcherTest
  • And others

The tests verify both logging functionality and log content through:

• Log message assertions
• Log count validations
• Log schema validations
• Log table content checks

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for logging-related test files and assertions in Spark tests

# Look for test files that might contain logging-related tests
rg -l "log.*Test" spark/src/test
rg -l "Log4j" spark/src/test

# Look for logging assertions or setup in test files
ast-grep --pattern 'assert($$$log$$$)' spark/src/test
ast-grep --pattern 'Logger.$$$' spark/src/test

Length of output: 168

---

Script:

#!/bin/bash
# Let's try a broader search for logging-related code in Spark tests and configuration

# Search for any logging-related imports or configurations
rg -l "import.*log" spark/src/test
rg -l "log4j" --ignore-case spark/src/test

# Check for logging configuration files
fd "log4j2?.properties|log4j2?.xml" spark/src

# Look for logging setup in test resources
fd "log.*properties|log.*xml" spark/src/test/resources

# Search for logging-related code with context
rg "log" -A 3 -B 3 spark/src/test

Length of output: 107782

---

Script:

#!/bin/bash
# Let's check for log4j2 configuration and test setup

# Look for log4j2 configuration files
fd "log4j2.*" spark/src

# Check for test configuration files that might set up logging
rg -l "log4j2?" spark/src/test/resources

# Look for logging configuration in test setup
rg "SparkSessionBuilder.*build" -A 5 -B 5 spark/src/test

# Check for any logging-related test assertions
rg "assert.*log" spark/src/test

Length of output: 45625

---

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

• Review comments: Directly reply to a review comment made by CodeRabbit. Example:
  • I pushed a fix in commit <commit_id>, please review it.
  • Generate unit testing code for this file.
  • Open a follow-up GitHub issue for this discussion.
• Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
  • @coderabbitai generate unit testing code for this file.
  • @coderabbitai modularize this function.
• PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
  • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
  • @coderabbitai read src/utils.ts and generate unit testing code.
  • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
  • @coderabbitai help me debug CodeRabbit configuration file.

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)

• @coderabbitai pause to pause the reviews on a PR.
• @coderabbitai resume to resume the paused reviews.
• @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
• @coderabbitai full review to do a full review from scratch and review all the files again.
• @coderabbitai summary to regenerate the summary of the PR.
• @coderabbitai resolve resolve all the CodeRabbit review comments.
• @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
• @coderabbitai help to get help.

Other keywords and placeholders

• Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
• Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
• Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

• You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
• Please see the configuration documentation for more information.
• If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

• Visit our Documentation for detailed information on how to use CodeRabbit.
• Join our Discord Community to get help, request features, and share feedback.
• Follow us on X/Twitter for updates and announcements.

[coderabbitai]

Actionable comments posted: 1

🧹 Outside diff range and nitpick comments (1)

build.sbt (1)

Line range hint 275-279: Inconsistent logging configuration in hub project

The hub project still overrides the logback version which conflicts with the PR objective of using log4j everywhere consistently. Consider:

1. Removing the logback override
2. Adding log4j2 dependencies to the hub project

     // Ensure consistent versions of logging libraries
     dependencyOverrides ++= Seq(
-      "org.slf4j" % "slf4j-api" % "1.7.36",
-      "ch.qos.logback" % "logback-classic" % "1.2.13"
+      "org.slf4j" % "slf4j-api" % "1.7.36"
     )

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between c3ec7d4 and 8c2b333.

📒 Files selected for processing (3)

• build.sbt (2 hunks)
• docker-init/demo/run.sh (1 hunks)
• spark/src/main/resources/log4j2.properties (1 hunks)

✅ Files skipped from review due to trivial changes (1)

• spark/src/main/resources/log4j2.properties

🔇 Additional comments (3)

docker-init/demo/run.sh (1)

Line range hint 24-33: Verify Log4j2 configuration pickup in Spark environment

Since this PR aims to implement consistent log4j usage, we should verify that Spark properly picks up the Log4j2 configuration without explicit configuration in spark-submit.

Let's verify the logging setup:

✅ Verification successful

Log4j2 configuration is properly set up in the Spark environment

The verification shows that Log4j2 is correctly configured:

• Log4j2 configuration files are present in both docker-init/demo/log4j2.properties and spark/src/main/resources/log4j2.properties
• Both configuration files have identical, proper Log4j2 settings including:
  • Root logger configuration
  • Console appender with pattern layout
  • Specific logger for ai.chronon package
  • ANSI color support enabled
• The configuration files are in the correct locations where Spark will automatically pick them up
• No explicit logging configurations are needed in spark-submit as the default classpath will include these configuration files

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for Log4j2 configuration files and related settings

# Check for Log4j2 configuration files in the codebase
echo "Searching for Log4j2 configuration files..."
fd -e properties -e xml -e yaml -e yml -e json | rg -i "log4j2"

# Check for logging-related configurations in Spark configuration files
echo "Checking Spark configuration files for logging settings..."
rg -l "log4j" . | rg -i "spark.*\.conf$"

# Check for logging-related dependencies in build files
echo "Checking build files for logging dependencies..."
rg -A 5 "log4j" . | rg "\.sbt$|pom\.xml$|build\.gradle$"

Length of output: 570

---

Script:

#!/bin/bash
# Let's examine the content of the log4j2 configuration files to verify their setup
echo "Content of docker-init/demo/log4j2.properties:"
cat docker-init/demo/log4j2.properties

echo -e "\nContent of spark/src/main/resources/log4j2.properties:"
cat spark/src/main/resources/log4j2.properties

# Check build files for Log4j2 dependencies
echo -e "\nChecking build files for Log4j2 dependencies:"
rg -i "log4j2" . | rg "\.sbt$|pom\.xml$|build\.gradle$|\.gradle$"

# Check if there are any explicit logging configurations in spark-submit scripts
echo -e "\nChecking spark-submit scripts for logging configurations:"
rg "spark-submit" -A 10 | rg -i "log"

Length of output: 5176

build.sbt (2)

170-174: LGTM! Comprehensive exclusion rules for logging conflicts

The exclusion rules correctly prevent multiple logging implementations from conflicting with Log4j2.

---

Line range hint 1-1: Verify complete removal of Logback configurations

Let's verify that all Logback configurations have been removed and Log4j2 is consistently used across all projects.

[nikhil-zlai]

very nice!