-
Notifications
You must be signed in to change notification settings - Fork 27
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
Change logging format to JSON #162
base: master
Are you sure you want to change the base?
Conversation
WalkthroughThe pull request modifies the logging configuration in the environment variable files Changes
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
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
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
docker/docker-compose.yaml (1)
Missing LOG_FORMAT configuration in some services
I found inconsistencies in the logging configuration across services:
- Main docker-compose.yaml shows
INDEXER_LOG_FORMAT: ${LOG_FORMAT}
for some services- However, docker-compose.dex.yaml services are missing the
INDEXER_LOG_FORMAT
configuration- All services use journald as the logging driver
Services needing LOG_FORMAT configuration:
- All services in docker/addons/docker-compose.dex.yaml that have
INDEXER_LOG_LEVEL
set🔗 Analysis chain
Line range hint
1-1
: Verify complete coverage of JSON logging formatSince this PR is marked as WIP, we should verify if any other services need the JSON logging format configuration.
Let's check for other services that might need logging configuration:
Let me check the content of these files to see if they have consistent logging configurations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find services that might need logging configuration # Look for services with logging-related configuration rg -l "LOG_LEVEL|logging:" --type yaml # Look for potential logging initialization in the codebase ast-grep --pattern 'log.$$$Format$$$'Length of output: 208
Script:
#!/bin/bash # Check logging configurations in all docker-compose files echo "=== docker/docker-compose.yaml ===" rg "LOG_LEVEL|logging:" -A 3 docker/docker-compose.yaml echo -e "\n=== docker/addons/docker-compose.dex.yaml ===" rg "LOG_LEVEL|logging:" -A 3 docker/addons/docker-compose.dex.yaml echo -e "\n=== docker/docker-compose.dev.yaml ===" rg "LOG_LEVEL|logging:" -A 3 docker/docker-compose.dev.yaml echo -e "\n=== docker/docker-compose.prod.yaml ===" rg "LOG_LEVEL|logging:" -A 3 docker/docker-compose.prod.yamlLength of output: 3985
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
docker/.env.mainnet.example
(1 hunks)docker/.env.testnet.example
(1 hunks)docker/docker-compose.yaml
(3 hunks)
🔇 Additional comments (3)
docker/.env.testnet.example (1)
43-43
: LGTM: JSON logging format configuration added
The addition of LOG_FORMAT="json"
is well-placed in the logging configuration section and uses a standard format.
Let's verify that this new environment variable is properly consumed by all relevant services:
✅ Verification successful
LGTM: LOG_FORMAT environment variable is properly consumed
The LOG_FORMAT
environment variable is correctly used by all three indexer services:
- indexer-exchange-import
- indexer-exchange-process
- indexer-exchange-api
Each service properly maps the variable to its internal INDEXER_LOG_FORMAT
configuration, ensuring consistent JSON logging format across the indexer stack.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify LOG_FORMAT usage across the codebase
# Check for any direct references to LOG_FORMAT
rg "LOG_FORMAT" --type yaml --type sh
Length of output: 238
Script:
#!/bin/bash
# Let's check the content of docker-compose.yaml to understand the services using LOG_FORMAT
cat docker/docker-compose.yaml
Length of output: 10989
docker/.env.mainnet.example (1)
43-43
: LGTM: Consistent JSON logging format across environments
The addition matches the testnet configuration, maintaining consistency between environments.
docker/docker-compose.yaml (1)
115-115
: LGTM: JSON logging format consistently applied
The INDEXER_LOG_FORMAT
environment variable is properly configured across all indexer services, correctly referencing the LOG_FORMAT
variable from the environment files.
Also applies to: 167-167, 255-255
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
docker/addons/docker-compose.dex.yaml (1)
152-152
: Remove trailing whitespace.There is a trailing whitespace after
${LOG_FORMAT}
that should be removed.- INDEXER_LOG_FORMAT: ${LOG_FORMAT} + INDEXER_LOG_FORMAT: ${LOG_FORMAT}🧰 Tools
🪛 yamllint (1.35.1)
[error] 152-152: trailing spaces
(trailing-spaces)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
docker/addons/docker-compose.dex.yaml
(7 hunks)
🧰 Additional context used
🪛 yamllint (1.35.1)
docker/addons/docker-compose.dex.yaml
[error] 152-152: trailing spaces
(trailing-spaces)
🔇 Additional comments (2)
docker/addons/docker-compose.dex.yaml (2)
20-20
: LGTM! Consistent logging format configuration across services.
The INDEXER_LOG_FORMAT
environment variable has been consistently added to all services, which aligns with the PR objective to change the logging format to JSON.
Also applies to: 93-93, 152-152, 221-221, 278-278, 341-341, 400-400
20-20
: Verify logging configuration.
Please ensure:
- The
${LOG_FORMAT}
variable is properly defined in the environment files. - The JSON log format works correctly with the journald logging driver.
Let's verify the environment configuration:
Also applies to: 93-93, 152-152, 221-221, 278-278, 341-341, 400-400
✅ Verification successful
LOG_FORMAT configuration is properly defined and compatible with journald
The verification shows that:
LOG_FORMAT
is properly defined in both example environment files (docker/.env.mainnet.example
anddocker/.env.testnet.example
) with a default value of "json"- All services are configured to use the journald logging driver, which is fully compatible with JSON log format
- The configuration is consistent across all services in both the main docker-compose file and the dex addon
The logging configuration is well-structured and follows best practices for containerized applications.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if LOG_FORMAT is defined in environment files and verify its value
# Check environment files for LOG_FORMAT definition
echo "Checking environment files for LOG_FORMAT..."
fd -t f "^\.env.*$" docker/ -x grep -l "LOG_FORMAT"
# Check the value of LOG_FORMAT
echo "Checking LOG_FORMAT value..."
fd -t f "^\.env.*$" docker/ -x grep "LOG_FORMAT"
# Check if any services are already using JSON logging with journald
echo "Checking for existing JSON logging configuration..."
rg -l "logging.*json" .
Length of output: 821
Script:
#!/bin/bash
# Let's modify our search to include hidden files and check docker-compose files for logging configuration
# Check all files in docker directory for LOG_FORMAT
echo "Checking all files in docker directory for LOG_FORMAT..."
rg "LOG_FORMAT" docker/
# Check docker-compose files for logging configuration
echo "Checking docker-compose files for logging configuration..."
fd -e yaml -e yml . docker/ -x grep -A 5 -B 5 "logging:"
# Check for environment files without dot prefix
echo "Checking for environment files..."
fd "env" docker/ -x cat {}
Length of output: 5844
Script:
#!/bin/bash
# Let's check for environment variable definitions in the repository root and docker directory
# Check for any file containing LOG_FORMAT definition
echo "Checking for LOG_FORMAT definition in any file..."
rg "LOG_FORMAT=" .
# Check specifically for environment files in the root
echo "Checking root directory environment files..."
ls -la .env* 2>/dev/null
# Check docker directory for any file that might define environment variables
echo "Checking docker directory for environment files..."
find docker -type f -exec grep -l "LOG_FORMAT=" {} \;
# Check if there's any documentation about logging configuration
echo "Checking for logging documentation..."
rg -i "log.*format|logging.*format" "README*"
Length of output: 648
Script:
#!/bin/bash
# Let's examine the example environment files found in the docker directory
# Check the content of example environment files
echo "Checking content of .env.mainnet.example..."
cat docker/.env.mainnet.example
echo -e "\nChecking content of .env.testnet.example..."
cat docker/.env.testnet.example
# Also check if there are any default values set in the compose files
echo -e "\nChecking compose files for default LOG_FORMAT values..."
rg "LOG_FORMAT.*json|LOG_FORMAT.*text" docker/
Length of output: 12028
Currently missing injective-core proper setting.
Summary by CodeRabbit
New Features
LOG_FORMAT
environment variable for enhanced logging capabilities.INDEXER_LOG_FORMAT
environment variable to multiple services for improved logging.Bug Fixes
INDEXER_SERVICE_WAIT_TIMEOUTEVEL
toINDEXER_SERVICE_WAIT_TIMEOUT
for better clarity.