fix: lower case stream name #4987
Conversation
|
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 (
|
hengfeiyang
force-pushed
the
fix/stream-name
branch
from
888062c to
95b277c
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
src/common/meta/organization.rs (1)
108-108: LGTM! Consider documenting the breaking change.The change to snake_case is consistent with Rust conventions and the previous trace_id change.
Consider adding a comment or updating documentation to note this breaking change, especially if these field names are part of your public API or used in queries.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
src/common/meta/organization.rs(1 hunks)src/config/src/utils/schema.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
src/common/meta/organization.rs (1)
Pattern **/*.rs: You are a smart rustlang pull request reviewer.
You are going to review all the rustlang files.
Be concise, and add a brief explanation to your suggestions
Make sure the responses are not too verbose and keep the suggestions to the point i.e. actionable.
src/config/src/utils/schema.rs (1)
Pattern **/*.rs: You are a smart rustlang pull request reviewer.
You are going to review all the rustlang files.
Be concise, and add a brief explanation to your suggestions
Make sure the responses are not too verbose and keep the suggestions to the point i.e. actionable.
🔇 Additional comments (2)
src/common/meta/organization.rs (1)
104-104: LGTM! Verify impact on dependent systems.
The change to snake_case follows Rust naming conventions. However, since this is a field name change, ensure all dependent systems (e.g., log ingestion, queries) are updated accordingly.
✅ Verification successful
Let me gather more information about the usage of the field name specifically, rather than just the type name.
Based on the search results, I can now generate the final response:
Change to trace_id is consistent with existing codebase usage
The codebase already predominantly uses trace_id (snake_case) in most places, including:
- Configuration (
src/config/src/config.rs) - HTTP handlers (
src/handler/http/request/*) - Metadata handling (
src/service/metadata/trace_list_index.rs)
The only exception is in src/service/logs/otlp_http.rs which handles both formats for compatibility:
if log.get("trace_id").is_some() {
// ...
} else if log.get("traceId").is_some() {
// ...
}This change aligns with the existing convention and maintains backward compatibility where needed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining usage of "traceId" to ensure consistent updates
rg -i "traceid" --type rust
Length of output: 2251
Script:
#!/bin/bash
# Search for field name usage patterns
rg -i '"trace_id"|"traceId"' --type rust -A 2
Length of output: 5485
src/config/src/utils/schema.rs (1)
266-266: LGTM! Consider adding tests for case sensitivity.
The addition of .to_lowercase() aligns with the PR objective. However, since this is a public function and the change affects string case sensitivity, we should add test cases to verify the behavior.
Let's verify if this change impacts any existing code:
Would you like me to help create test cases for the format_stream_name function? I can suggest test cases covering:
- Mixed case input
- Special characters with mixed case
- Numbers with mixed case
✅ Verification successful
Lowercasing stream names is safe and consistent with existing usage
The verification shows that format_stream_name is used consistently across the codebase for normalizing stream names. The change to lowercase stream names is safe because:
- All usages of
format_stream_nameare for normalization purposes, not for display or comparison with hardcoded values - Stream names in SQL queries and constants are already using lowercase
- The function is used at ingestion/creation time, ensuring consistent casing throughout the data lifecycle
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for direct usage of format_stream_name
rg "format_stream_name\(" --type rust
# Search for any hardcoded stream names that might rely on case sensitivity
rg -i "stream_name.*=.*[A-Z]" --type rust
Length of output: 17600
fixed #4986 #4979
Summary by CodeRabbit
New Features
Bug Fixes