impl(otel): copy service resource labels into metric labels by IcySteam · Pull Request #14825 · googleapis/google-cloud-cpp · GitHub

This change is

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 ?

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.

/gcbrun

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

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.

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.

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

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()

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

We probably want to use GMock's container matchers.

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

ifs in tests kind of scare me.

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.

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 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

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

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

+                  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) ==

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

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

               // See: https://cloud.google.com/monitoring/quotas
               auto constexpr kMaxTimeSeriesPerRequest = 200;
+              std::unordered_map<std::string, OTelKeyMatch> const kExtraLabelsLookup = {

                     }
                   }
                 }
+                WithExtraLabels(data, tss);

impl(otel): copy service resource labels into metric labels by IcySteam · Pull Request #14825 · googleapis/google-cloud-cpp · GitHub

Uh oh!

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

impl(otel): copy service resource labels into metric labels#14825IcySteam wants to merge 3 commits intogoogleapis:maingoogleapis/google-cloud-cpp:mainfrom IcySteam:otel-servicelabelsIcySteam/google-cloud-cpp:otel-servicelabelsCopy head branch name to clipboard

Conversation

IcySteam commented Nov 6, 2024 • edited by scotthart Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dbolduc left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dbolduc commented Nov 22, 2024

Uh oh!

codecov Bot commented Nov 22, 2024

Codecov Report

Uh oh!

IcySteam commented Nov 25, 2024

Uh oh!

dbolduc left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dbolduc commented Jan 10, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

impl(otel): copy service resource labels into metric labels#14825
IcySteam wants to merge 3 commits into
googleapis:mainfrom
IcySteam:otel-servicelabels

IcySteam commented Nov 6, 2024 •
edited by scotthart
Loading