add more logging in app-server startup #443
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
❌ Changes requested. Reviewed everything up to 279655d in 1 minute and 47 seconds
More details
- Looked at
201lines of code in1files - Skipped
0files when reviewing. - Skipped posting
14drafted comments based on config settings.
1. app-server/src/main.rs:106
- Draft comment:
Consider checking that the RUST_LOG value is non-empty, as an empty string will pass the is_ok() check but may not set the desired logging filter. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50%
The comment raises a valid point - an empty RUST_LOG value would pass is_ok() but may not set the desired logging filter. However, this is a minor edge case. The code already has a reasonable fallback (LevelFilter::Info) if RUST_LOG is not set. An empty RUST_LOG value would just use Rust's default logging behavior which is acceptable.
The comment identifies a real edge case that could affect logging behavior. Empty environment variables are a common source of bugs.
While technically correct, this is an edge case with minimal impact. The logging system will still work with reasonable defaults even with an empty RUST_LOG. This doesn't meet the bar of being an important enough issue to flag.
Delete the comment. While technically valid, this is too minor of an edge case to warrant a code change, especially since the logging system has reasonable fallback behavior.
2. app-server/src/main.rs:119
- Draft comment:
Logging the HTTP payload limit adds clarity on configuration. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
3. app-server/src/main.rs:141
- Draft comment:
Good to log the Redis URL; this helps in troubleshooting cache configuration. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
4. app-server/src/main.rs:217
- Draft comment:
Logging RabbitMQ channel pool size for spans is useful. Ensure that error handling for parsing the env var is sufficient. - Reason this comment was not posted:
Confidence changes required:20%<= threshold50%
None
5. app-server/src/main.rs:258
- Draft comment:
Logging the browser events channel size is clear. Consider consistent phrasing between different queue types. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
6. app-server/src/main.rs:291
- Draft comment:
Logging which storage (S3 vs mock) is used aids debugging. The log messages clearly indicate the chosen implementation. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
7. app-server/src/main.rs:338
- Draft comment:
Logging the Machine Manager URL and fallback to mock is clear for troubleshooting external dependencies. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
8. app-server/src/main.rs:360
- Draft comment:
Logging the Agent Manager URL is useful for verifying configuration. The fallback log enhances clarity. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
9. app-server/src/main.rs:488
- Draft comment:
Logging the worker counts for spans and browser events is a helpful addition. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
10. app-server/src/main.rs:405
- Draft comment:
Logging the Code Executor URL and fallback to mock provides clear visibility of external service configuration. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
11. app-server/src/main.rs:119
- Draft comment:
Good addition logging the HTTP and gRPC payload limits to improve observability. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
12. app-server/src/main.rs:217
- Draft comment:
Logging the RabbitMQ channel pool size is useful. Ensure that the fallback default (64) is appropriate for all environments. - Reason this comment was not posted:
Confidence changes required:20%<= threshold50%
None
13. app-server/src/main.rs:343
- Draft comment:
Several service URLs (Machine Manager, Agent Manager, Semantic Search, Code Executor) are logged. Ensure these logs do not expose sensitive details (e.g. embedded passwords or tokens) in production. - Reason this comment was not posted:
Comment was on unchanged code.
14. app-server/src/main.rs:477
- Draft comment:
Nice improvement: logging the number of span and browser event workers clarifies configuration. Moving these variable declarations outside the HttpServer closure aids readability. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
Workflow ID: wflow_1ls92xp3qtScCdrY
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
app-server/src/main.rs
Outdated
| @@ -103,19 +103,28 @@ fn main() -> anyhow::Result<()> { | |||
|
|
|||
| let mut handles: Vec<JoinHandle<Result<(), Error>>> = vec![]; | |||
|
|
|||
| std::env::set_var("RUST_LOG", "info"); | |||
| env_logger::init(); | |||
| if env::var("RUST_LOG").is_ok() { | |||
There was a problem hiding this comment.
Note that if RUST_LOG is set to an empty string, it still returns Ok. Consider checking for an empty value or using a more robust initialization (e.g., init_from_env) so the default filter isn’t bypassed unintentionally.
Suggested change
| if env::var("RUST_LOG").is_ok() { | |
| if env::var("RUST_LOG").map(|val| !val.is_empty()).unwrap_or(false) { |
app-server/src/main.rs
Outdated
| @@ -129,11 +138,13 @@ fn main() -> anyhow::Result<()> { | |||
| // == Stuff that is needed both for HTTP and gRPC servers == | |||
| // === 1. Cache === | |||
| let cache = if let Ok(redis_url) = env::var("REDIS_URL") { | |||
| log::info!("Redis cache URL: {}", redis_url); | |||
There was a problem hiding this comment.
Logging the Redis cache URL can expose sensitive connection details. Consider redacting credentials if present.
Suggested change
| log::info!("Redis cache URL: {}", redis_url); | |
| log::info!("Redis cache URL: {}", redis_url.replace(&redis_url[redis_url.find('@').unwrap_or(0)..], "<redacted>")); |
There was a problem hiding this comment.
❌ Changes requested. Incremental review on 279655d in 1 minute and 43 seconds
More details
- Looked at
201lines of code in1files - Skipped
0files when reviewing. - Skipped posting
7drafted comments based on config settings.
1. app-server/src/main.rs:106
- Draft comment:
Ensure env_logger is initialized only once. Double-check that no duplicate init calls occur in different modules. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
2. app-server/src/main.rs:147
- Draft comment:
Use consistent capitalization in log messages (e.g., 'Using In-Memory Cache' rather than 'using in-memory cache'). - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
3. app-server/src/main.rs:106
- Draft comment:
Consider verifying not only the existence of RUST_LOG but also that it isn’t empty. An empty string might bypass the intended filtering behavior. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
4. app-server/src/main.rs:141
- Draft comment:
Logging the Redis cache URL could inadvertently expose sensitive credentials. Consider redacting passwords or sensitive parts in production logs. - Reason this comment was not posted:
Marked as duplicate.
5. app-server/src/main.rs:343
- Draft comment:
Multiple service URLs (e.g. machine manager, agent manager, semantic search, and code executor) are now logged. Ensure that these endpoints do not include sensitive tokens or credentials, or that they are properly sanitized for production. - Reason this comment was not posted:
Comment was on unchanged code.
6. app-server/src/main.rs:487
- Draft comment:
Good job extracting and logging the worker counts for spans and browser events. Consider adding documentation or validation for allowed ranges, to preempt misconfiguration. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
7. app-server/src/main.rs:291
- Draft comment:
For consistency, consider unifying the capitalization and style of log messages (e.g. 'using S3 storage' vs 'using mock storage'). - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
Workflow ID: wflow_QA1aOmKTMQKYVgsZ
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
app-server/src/main.rs
Outdated
| @@ -129,11 +138,13 @@ fn main() -> anyhow::Result<()> { | |||
| // == Stuff that is needed both for HTTP and gRPC servers == | |||
| // === 1. Cache === | |||
| let cache = if let Ok(redis_url) = env::var("REDIS_URL") { | |||
| log::info!("Redis cache URL: {}", redis_url); | |||
There was a problem hiding this comment.
Avoid logging potentially sensitive information like connection URLs. Consider redacting credentials in REDIS_URL.
Suggested change
| log::info!("Redis cache URL: {}", redis_url); | |
| log::info!("Redis cache URL: {}", redact_credentials(redis_url)); |
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on a87e29f in 34 seconds
More details
- Looked at
22lines of code in1files - Skipped
0files when reviewing. - Skipped posting
4drafted comments based on config settings.
1. app-server/src/main.rs:106
- Draft comment:
Use of is_ok_and ensures RUST_LOG isn’t an empty string. Verify that the minimum Rust version supports is_ok_and. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%
None
2. app-server/src/main.rs:141
- Draft comment:
Switched logging from showing the Redis URL to a generic message, likely to avoid exposing sensitive data. Ensure this change meets your debugging needs. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%
None
3. app-server/src/main.rs:106
- Draft comment:
Good improvement checking for non-empty RUST_LOG. Ensure your minimum Rust version supports is_ok_and, or consider an alternative if backward compatibility is needed. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
4. app-server/src/main.rs:141
- Draft comment:
Logging now omits the Redis URL. This improves security, but may hinder debugging. Consider logging non-sensitive parts or using a debug log level. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
Workflow ID: wflow_f22pjTNbWl0yotRy
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 4b77f90 in 49 seconds
More details
- Looked at
15lines of code in1files - Skipped
0files when reviewing. - Skipped posting
4drafted comments based on config settings.
1. README.md:54
- Draft comment:
Clarify 'baseUrl' usage - ensure the parameter and config instructions are up-to-date with docs. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
2. README.md:54
- Draft comment:
SDK configuration instructions are helpful. Consider adding a brief example or mentioning default port values to improve clarity. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
3. README.md:25
- Draft comment:
The checklist item on line 25 uses an uppercase 'X' while the rest of the checkboxes use lowercase 'x'. Consider using a consistent style (e.g., [x]) for improved readability. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
4. README.md:3
- Draft comment:
There's an extra space between the opening tag and the image markdown in line 3. Removing this extra space will help keep the formatting consistent. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_GGkd1hF0wtAcWn9N
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Important
Add detailed logging to
app-server/src/main.rsfor configuration and runtime decisions, and updateREADME.mdwith SDK setup instructions.main.rs.main.rs.main.rs.main.rs.main.rs.README.mdto include SDK configuration instructions.This description was created by
for 4b77f90. It will automatically update as commits are pushed.