add tenant id to metrics aggregation#1568
Open
nikhilsinhaparseable wants to merge 2 commits intoparseablehq:mainfrom
Open
add tenant id to metrics aggregation#1568nikhilsinhaparseable wants to merge 2 commits intoparseablehq:mainfrom
nikhilsinhaparseable wants to merge 2 commits intoparseablehq:mainfrom
Conversation
Contributor
WalkthroughAdded tenant-scoped billing metadata and per-tenant metric collection: billing event and collector structs now carry an optional tenant_id; Prometheus sample extraction groups samples per-tenant and instantiates per-tenant collectors. Storage metadata and tenant metadata APIs were extended to store and update billing-related tenant fields. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/handlers/http/cluster/mod.rs`:
- Around line 1397-1402: Replace the hardcoded string "DEFAULT_TENANT" with the
shared constant from crate::parseable: import or reference
crate::parseable::DEFAULT_TENANT and use it in the closure inside the filter on
sample.labels.get("tenant_id") so the tenant_id extraction uses DEFAULT_TENANT
instead of the literal; update the filter to compare against DEFAULT_TENANT
(e.g., .filter(|t| *t != DEFAULT_TENANT)) while keeping the surrounding logic
that maps to Option<String>.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 0e6ee6ec-7df2-4c5c-9844-a5025ed53076
📒 Files selected for processing (1)
src/handlers/http/cluster/mod.rs
Comment on lines
+1397
to
+1402
| // Extract tenant_id from labels; treat "DEFAULT_TENANT" or absent as None (single-tenant compat) | ||
| let tenant_id = sample | ||
| .labels | ||
| .get("tenant_id") | ||
| .filter(|t| *t != "DEFAULT_TENANT") | ||
| .map(|t| t.to_string()); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Find where DEFAULT_TENANT is defined and its value
echo "=== DEFAULT_TENANT definition ==="
rg -n --type=rust 'const\s+DEFAULT_TENANT|pub\s+const\s+DEFAULT_TENANT|DEFAULT_TENANT\s*='
# 2) Read the specific lines in cluster/mod.rs to see the hardcoded string
echo -e "\n=== Code at lines 1397-1402 in cluster/mod.rs ==="
sed -n '1390,1410p' src/handlers/http/cluster/mod.rs
# 3) Check current imports in cluster/mod.rs
echo -e "\n=== Imports in cluster/mod.rs ==="
head -50 src/handlers/http/cluster/mod.rs | grep -E '^use|^import'Repository: parseablehq/parseable
Length of output: 2171
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 4) Check how DEFAULT_TENANT is used in other tenant normalization contexts
echo "=== DEFAULT_TENANT usage patterns in codebase ==="
rg -B2 -A2 'DEFAULT_TENANT' --type=rust | head -100
# 5) Search for other similar tenant_id normalizations
echo -e "\n=== Other tenant_id label/filter patterns ==="
rg -n 'tenant_id.*filter|filter.*tenant_id' --type=rust src/Repository: parseablehq/parseable
Length of output: 8066
Use the DEFAULT_TENANT constant instead of hardcoding the string literal.
Line 1401 hardcodes "DEFAULT_TENANT" in the filter. To maintain consistency with the codebase and prevent normalization drift if the constant changes, use the shared constant from crate::parseable.
♻️ Proposed change
-use crate::parseable::PARSEABLE;
+use crate::parseable::{DEFAULT_TENANT, PARSEABLE};
...
- .filter(|t| *t != "DEFAULT_TENANT")
+ .filter(|t| t.as_str() != DEFAULT_TENANT)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Extract tenant_id from labels; treat "DEFAULT_TENANT" or absent as None (single-tenant compat) | |
| let tenant_id = sample | |
| .labels | |
| .get("tenant_id") | |
| .filter(|t| *t != "DEFAULT_TENANT") | |
| .map(|t| t.to_string()); | |
| // Extract tenant_id from labels; treat "DEFAULT_TENANT" or absent as None (single-tenant compat) | |
| let tenant_id = sample | |
| .labels | |
| .get("tenant_id") | |
| .filter(|t| t.as_str() != DEFAULT_TENANT) | |
| .map(|t| t.to_string()); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/handlers/http/cluster/mod.rs` around lines 1397 - 1402, Replace the
hardcoded string "DEFAULT_TENANT" with the shared constant from
crate::parseable: import or reference crate::parseable::DEFAULT_TENANT and use
it in the closure inside the filter on sample.labels.get("tenant_id") so the
tenant_id extraction uses DEFAULT_TENANT instead of the literal; update the
filter to compare against DEFAULT_TENANT (e.g., .filter(|t| *t !=
DEFAULT_TENANT)) while keeping the surrounding logic that maps to
Option<String>.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/tenants/mod.rs`:
- Around line 66-83: The update_tenant_meta function currently overwrites
tenant.meta fields with whichever Option<String> values are passed, clobbering
existing metadata when callers pass None; change the logic in update_tenant_meta
so that for each incoming parameter (customer_name, start_date, end_date, plan)
you only assign to tenant.meta.<field> when the corresponding Option is
Some(value) and leave the existing value untouched when it is None, and if you
need to support explicit clearing distinguish "unset" vs "clear" at the API
boundary by using Option<Option<String>> instead.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: a3e5d05a-8487-4593-82bd-00187c0b9066
📒 Files selected for processing (2)
src/storage/store_metadata.rssrc/tenants/mod.rs
Comment on lines
+66
to
+83
| pub fn update_tenant_meta( | ||
| &self, | ||
| tenant_id: &str, | ||
| customer_name: Option<String>, | ||
| start_date: Option<String>, | ||
| end_date: Option<String>, | ||
| plan: Option<String>, | ||
| ) -> bool { | ||
| if let Some(mut tenant) = self.tenants.get_mut(tenant_id) { | ||
| tenant.meta.customer_name = customer_name; | ||
| tenant.meta.start_date = start_date; | ||
| tenant.meta.end_date = end_date; | ||
| tenant.meta.plan = plan; | ||
| true | ||
| } else { | ||
| false | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Avoid clobbering existing metadata on partial updates.
On Line 75–78, assigning each field directly from Option<String> erases existing values whenever callers pass None (including omitted fields in patch-like APIs). That can cause silent metadata loss.
💡 Proposed fix
pub fn update_tenant_meta(
&self,
tenant_id: &str,
customer_name: Option<String>,
start_date: Option<String>,
end_date: Option<String>,
plan: Option<String>,
) -> bool {
if let Some(mut tenant) = self.tenants.get_mut(tenant_id) {
- tenant.meta.customer_name = customer_name;
- tenant.meta.start_date = start_date;
- tenant.meta.end_date = end_date;
- tenant.meta.plan = plan;
+ if let Some(customer_name) = customer_name {
+ tenant.meta.customer_name = Some(customer_name);
+ }
+ if let Some(start_date) = start_date {
+ tenant.meta.start_date = Some(start_date);
+ }
+ if let Some(end_date) = end_date {
+ tenant.meta.end_date = Some(end_date);
+ }
+ if let Some(plan) = plan {
+ tenant.meta.plan = Some(plan);
+ }
true
} else {
false
}
}If clearing fields is also required, use Option<Option<String>> at the API boundary to distinguish “unset” vs “explicitly clear”.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| pub fn update_tenant_meta( | |
| &self, | |
| tenant_id: &str, | |
| customer_name: Option<String>, | |
| start_date: Option<String>, | |
| end_date: Option<String>, | |
| plan: Option<String>, | |
| ) -> bool { | |
| if let Some(mut tenant) = self.tenants.get_mut(tenant_id) { | |
| tenant.meta.customer_name = customer_name; | |
| tenant.meta.start_date = start_date; | |
| tenant.meta.end_date = end_date; | |
| tenant.meta.plan = plan; | |
| true | |
| } else { | |
| false | |
| } | |
| } | |
| pub fn update_tenant_meta( | |
| &self, | |
| tenant_id: &str, | |
| customer_name: Option<String>, | |
| start_date: Option<String>, | |
| end_date: Option<String>, | |
| plan: Option<String>, | |
| ) -> bool { | |
| if let Some(mut tenant) = self.tenants.get_mut(tenant_id) { | |
| if let Some(customer_name) = customer_name { | |
| tenant.meta.customer_name = Some(customer_name); | |
| } | |
| if let Some(start_date) = start_date { | |
| tenant.meta.start_date = Some(start_date); | |
| } | |
| if let Some(end_date) = end_date { | |
| tenant.meta.end_date = Some(end_date); | |
| } | |
| if let Some(plan) = plan { | |
| tenant.meta.plan = Some(plan); | |
| } | |
| true | |
| } else { | |
| false | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/tenants/mod.rs` around lines 66 - 83, The update_tenant_meta function
currently overwrites tenant.meta fields with whichever Option<String> values are
passed, clobbering existing metadata when callers pass None; change the logic in
update_tenant_meta so that for each incoming parameter (customer_name,
start_date, end_date, plan) you only assign to tenant.meta.<field> when the
corresponding Option is Some(value) and leave the existing value untouched when
it is None, and if you need to support explicit clearing distinguish "unset" vs
"clear" at the API boundary by using Option<Option<String>> instead.
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Summary by CodeRabbit
Refactor
New Features