Conversation
There was a problem hiding this comment.
Code Review
This pull request adds the project_id attribute to the monitored resource labels in resource_to_monitored_resource and updates the corresponding unit and mock tests to assert its presence. Feedback suggests refactoring the match block in exporter.rs to reuse the existing is_monitored_resource_label helper function, which reduces duplication and improves maintainability.
| match key_str { | ||
| "instance_id" | "location" | "instance_config" | "client_hash" => { | ||
| "project_id" | "instance_id" | "location" | "instance_config" | "client_hash" => { | ||
| labels.insert(key_str.to_string(), value_to_string(val)); | ||
| } | ||
| _ => {} |
There was a problem hiding this comment.
Instead of duplicating the list of monitored resource labels here, we can reuse the existing helper function is_monitored_resource_label which is already defined in this file. This reduces duplication and prevents future inconsistencies.
if is_monitored_resource_label(key_str) {
labels.insert(key_str.to_string(), value_to_string(val));
}There was a problem hiding this comment.
this makes a lot of sense to me. Any reason not to reason the method @olavloite ?
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6843 +/- ##
==========================================
- Coverage 96.92% 96.91% -0.01%
==========================================
Files 327 327
Lines 108648 108651 +3
==========================================
- Hits 105309 105302 -7
- Misses 3339 3349 +10 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
alvarowolfx
left a comment
There was a problem hiding this comment.
gonna approve it, but ideally it should be refactored to use is_monitored_resource_label. Also you can use impl instead of chore, since you don't consider this a fix and metrics is still not available.
Adds the missing
project_idlabel to metrics.Note: Added as a 'chore:' and not 'fix:' as these metrics had not yet been released.