impl(otel): copy service resource labels into metric labels #14825

IcySteam · 2024-11-06T03:09:51Z

This change is

dbolduc

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 ?

dbolduc · 2024-11-13T19:15:14Z

google/cloud/opentelemetry/internal/monitored_resource.cc

+         scope_metrics.metric_data_) {
+      for (opentelemetry::sdk::metrics::PointDataAttributes& pda :
+           metric.point_data_attr_) {
+        auto& attributes = pda.attributes;


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.

dbolduc · 2024-11-22T16:54:19Z

/gcbrun

codecov · 2024-11-22T17:50:41Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.06%. Comparing base (aa062cf) to head (1775a3e).
Report is 27 commits behind head on main.

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.
📢 Have feedback on the report? Share it here.

🚨 Try these New Features:

Flaky Tests Detection - Detect and resolve failed and flaky tests

IcySteam · 2024-11-25T06:37:56Z

Thanks; we indeed want the labels to end up getting added to the google.api.Metric class. I've refactored the PR and added some unit tests in the latest commit, PTAL.

dbolduc

Thanks for the PR. Sorry it took me so long to review it. I have been splitting time between google-cloud-cpp and another team.

I feel like I owe you one, so I made the review changes in #14930. Hopefully that can cut out some review iterations and unblock y'all faster.

dbolduc · 2025-01-10T17:53:46Z

google/cloud/opentelemetry/internal/monitored_resource.h


 namespace google {
 namespace cloud {
 namespace otel_internal {
 GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_BEGIN

+struct OTelKeyMatch {
+  std::vector<std::string> otel_keys;
+  absl::optional<std::string> fallback = absl::nullopt;


FYI: I recommended that this struct change in #14923. So maybe wait for that PR and then rebase.

dbolduc · 2025-01-10T17:57:31Z

google/cloud/opentelemetry/internal/time_series.h

@@ -34,6 +38,15 @@ GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_BEGIN
 // See: https://cloud.google.com/monitoring/quotas
 auto constexpr kMaxTimeSeriesPerRequest = 200;

+std::unordered_map<std::string, OTelKeyMatch> const kExtraLabelsLookup = {


Google style guide frowns upon static variables that are not trivially destructible.

We have to use:

auto const* const kExtraLabelsLookup = new std::unordered_map<...> { ... };

On second thought, I don't think this struct is buying us much of anything. It seems kind of misapplied, in that the things like "service_name" are supposed to represent monitored resources.

I think we will be better off just iterating over the 3 labels we care about, and reuse the name changing logic from ToMetric()

dbolduc · 2025-01-10T22:40:04Z

google/cloud/opentelemetry/internal/time_series_test.cc

+  for (auto const& kv : kExtraLabelsLookup) {
+    auto const& oks = kv.second.otel_keys;
+    if (oks.empty()) {
+      continue;
+    }
+    for (auto const& ts : tss) {


We should avoid logical branches and stuff in tests. Let's try to make them as simple as possible.

dbolduc · 2025-01-10T22:40:56Z

google/cloud/opentelemetry/internal/time_series_test.cc

+      auto const& labels = ts.metric().labels();
+      ASSERT_TRUE(labels.find(kv.first) != labels.end());
+      EXPECT_EQ(labels.at(kv.first), kv.first);


We probably want to use GMock's container matchers.

https://google.github.io/googletest/reference/matchers.html

dbolduc · 2025-01-10T22:41:29Z

google/cloud/opentelemetry/internal/time_series_test.cc

+    for (auto const& ts : tss) {
+      auto const& labels = ts.metric().labels();
+      ASSERT_TRUE(labels.find(kv.first) != labels.end());
+      if (existing_labels.GetAttributes().find(kv.first) ==


ifs in tests kind of scare me.

dbolduc · 2025-01-10T22:49:23Z

google/cloud/opentelemetry/internal/time_series.cc

@@ -215,6 +217,7 @@ std::vector<google::monitoring::v3::TimeSeries> ToTimeSeries(
      }
    }
  }
+  WithExtraLabels(data, tss);


I think it would make sense to build this into ToMetric(...) which deals specifically with the google.api.Metric. It also has logic for changing strings from service.name -> service_name which we can reuse.

dbolduc · 2025-01-10T22:54:45Z

I am closing this PR in favor of #14930

Thank you @IcySteam for the hard work on this PR. I copied a lot of it in #14930.

IcySteam requested a review from a team as a code owner November 6, 2024 03:09

IcySteam marked this pull request as draft November 12, 2024 04:14

dbolduc requested changes Nov 13, 2024

View reviewed changes

dbolduc reviewed Nov 13, 2024

View reviewed changes

IcySteam force-pushed the otel-servicelabels branch from 7c4f06b to a5e0212 Compare November 22, 2024 12:00

impl(otel): copy service resource labels into metric labels

1775a3e

IcySteam force-pushed the otel-servicelabels branch from a5e0212 to 1775a3e Compare November 22, 2024 12:07

add tests

b17d474

IcySteam force-pushed the otel-servicelabels branch from ef964b2 to b17d474 Compare November 25, 2024 06:29

IcySteam marked this pull request as ready for review November 25, 2024 06:35

change WithExtraLabel's return type to void

95a35f5

dbolduc mentioned this pull request Jan 10, 2025

feat(otel): copy service labels into GCM Metric #14930

Merged

dbolduc reviewed Jan 10, 2025

View reviewed changes

dbolduc closed this Jan 10, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

impl(otel): copy service resource labels into metric labels #14825

impl(otel): copy service resource labels into metric labels #14825

IcySteam commented Nov 6, 2024 •

edited by scotthart

Loading

dbolduc left a comment

dbolduc Nov 13, 2024

dbolduc commented Nov 22, 2024

codecov bot commented Nov 22, 2024

IcySteam commented Nov 25, 2024

dbolduc left a comment

dbolduc Jan 10, 2025

dbolduc Jan 10, 2025

dbolduc Jan 10, 2025

dbolduc Jan 10, 2025

dbolduc Jan 10, 2025

dbolduc Jan 10, 2025

dbolduc Jan 10, 2025

dbolduc commented Jan 10, 2025

	for (auto& label : mr.labels) {
	(*resource.mutable_labels())[std::move(label.first)] =
	std::move(label.second);
	}

impl(otel): copy service resource labels into metric labels #14825

impl(otel): copy service resource labels into metric labels #14825

Conversation

IcySteam commented Nov 6, 2024 • edited by scotthart Loading

dbolduc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbolduc commented Nov 22, 2024

codecov bot commented Nov 22, 2024

Codecov Report

IcySteam commented Nov 25, 2024

dbolduc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbolduc commented Jan 10, 2025

IcySteam commented Nov 6, 2024 •

edited by scotthart

Loading