-
Notifications
You must be signed in to change notification settings - Fork 375
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
impl(otel): copy service resource labels into metric labels #14825
base: main
Are you sure you want to change the base?
Conversation
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.
I don't think the copying is necessary. Can we check for the known service labels and do the manipulations here:
google-cloud-cpp/google/cloud/opentelemetry/internal/time_series.cc
Lines 174 to 177 in 912533c
for (auto& label : mr.labels) { | |
(*resource.mutable_labels())[std::move(label.first)] = | |
std::move(label.second); | |
} |
and add a TEST(ToMonitoredResource, AddsServiceLabels) { ... }
in time_series_test.cc
?
scope_metrics.metric_data_) { | ||
for (opentelemetry::sdk::metrics::PointDataAttributes& pda : | ||
metric.point_data_attr_) { | ||
auto& attributes = pda.attributes; |
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.
Sorry, I am confused. Do we want the labels to end up getting added to the google.api.Metric
https://github.com/googleapis/googleapis/blob/2d05911be5a468b556236bee537c91922f9c23a3/google/api/metric.proto#L286
... or to the google.api.MonitoredResource
https://github.com/googleapis/googleapis/blob/2d05911be5a468b556236bee537c91922f9c23a3/google/api/monitored_resource.proto#L106
... or both?
unit tests would help clarify the intended behavior that we want.
7c4f06b
to
a5e0212
Compare
a5e0212
to
1775a3e
Compare
/gcbrun |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #14825 +/- ##
=======================================
Coverage 93.06% 93.06%
=======================================
Files 2319 2319
Lines 209023 209023
=======================================
+ Hits 194529 194530 +1
+ Misses 14494 14493 -1 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
ef964b2
to
b17d474
Compare
Thanks; we indeed want the labels to end up getting added to the |
fixed #14823
This change is