Skip to content

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

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.

Sign up for GitHub

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

Closed
IcySteam wants to merge 3 commits into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1775a3e
impl(otel): copy service resource labels into metric labels
IcySteam Nov 6, 2024
b17d474
add tests
IcySteam Nov 25, 2024
95a35f5
change WithExtraLabel's return type to void
IcySteam Nov 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 0 additions & 5 deletions google/cloud/opentelemetry/internal/monitored_resource.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,11 +49,6 @@ struct AsStringVisitor {
std::string operator()(bool const& v) const { return v ? "true" : "false"; }
};

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

class MonitoredResourceProvider {
public:
MonitoredResourceProvider(
Expand Down
7 changes: 7 additions & 0 deletions google/cloud/opentelemetry/internal/monitored_resource.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,16 +15,23 @@
#ifndef GOOGLE_CLOUD_CPP_GOOGLE_CLOUD_OPENTELEMETRY_INTERNAL_MONITORED_RESOURCE_H
#define GOOGLE_CLOUD_CPP_GOOGLE_CLOUD_OPENTELEMETRY_INTERNAL_MONITORED_RESOURCE_H

#include "absl/types/optional.h"
#include "google/cloud/version.h"
#include <opentelemetry/sdk/resource/resource.h>
#include <string>
#include <unordered_map>
#include <vector>

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;
Copy link
Member

@dbolduc dbolduc Jan 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

};

std::string AsString(
opentelemetry::sdk::common::OwnedAttributeValue const& attribute);

Expand Down
38 changes: 38 additions & 0 deletions google/cloud/opentelemetry/internal/time_series.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@
#include <opentelemetry/sdk/metrics/data/metric_data.h>
#include <opentelemetry/sdk/metrics/export/metric_producer.h>
#include <cctype>
#include <string>
#include <vector>

namespace google {
namespace cloud {
Expand Down Expand Up @@ -215,6 +217,7 @@ std::vector<google::monitoring::v3::TimeSeries> ToTimeSeries(
}
}
}
WithExtraLabels(data, tss);
Copy link
Member

@dbolduc dbolduc Jan 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return tss;
}

Expand All @@ -236,6 +239,41 @@ std::vector<google::monitoring::v3::CreateTimeSeriesRequest> ToRequests(
return requests;
}

void WithExtraLabels(
opentelemetry::sdk::metrics::ResourceMetrics const& data,
std::vector<google::monitoring::v3::TimeSeries>& tss,
std::unordered_map<std::string, OTelKeyMatch> const& extra_labels) {
if (!data.resource_) {
return;
}

opentelemetry::sdk::resource::ResourceAttributes const& attributes =
data.resource_->GetAttributes();
for (auto const& kv : extra_labels) {
auto const& oks = kv.second.otel_keys;
auto found = std::find_first_of(
oks.begin(), oks.end(), attributes.begin(), attributes.end(),
[](auto const& key, auto const& attr) { return key == attr.first; });

std::string value;
if (found != oks.end()) {
value = AsString(attributes.at(*found));
} else if (kv.second.fallback) {
value = *kv.second.fallback;
}
if (value.empty()) {
continue;
}

for (auto& ts : tss) {
auto& labels = *((*ts.mutable_metric()).mutable_labels());
if (labels.find(kv.first) == labels.end()) {
labels[kv.first] = value;
}
}
}
}

GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_END
} // namespace otel_internal
} // namespace cloud
Expand Down
28 changes: 28 additions & 0 deletions google/cloud/opentelemetry/internal/time_series.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,14 +15,18 @@
#ifndef GOOGLE_CLOUD_CPP_GOOGLE_CLOUD_OPENTELEMETRY_INTERNAL_TIME_SERIES_H
#define GOOGLE_CLOUD_CPP_GOOGLE_CLOUD_OPENTELEMETRY_INTERNAL_TIME_SERIES_H

#include "google/cloud/opentelemetry/internal/monitored_resource.h"
#include "google/cloud/version.h"
#include "absl/types/optional.h"
#include <google/api/metric.pb.h>
#include <google/api/monitored_resource.pb.h>
#include <google/monitoring/v3/metric_service.pb.h>
#include <opentelemetry/sdk/metrics/metric_reader.h>
#include <opentelemetry/sdk/resource/resource.h>
#include <opentelemetry/sdk/resource/semantic_conventions.h>
#include <functional>
#include <string>
#include <unordered_map>

namespace google {
namespace cloud {
Expand All @@ -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 = {
Copy link
Member

@dbolduc dbolduc Jan 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

We have to use:

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

Copy link
Member

@dbolduc dbolduc Jan 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

{"service_name",
{{opentelemetry::sdk::resource::SemanticConventions::kServiceName}}},
{"service_namespace",
{{opentelemetry::sdk::resource::SemanticConventions::kServiceNamespace}}},
{"service_instance_id",
{{opentelemetry::sdk::resource::SemanticConventions::
kServiceInstanceId}}}};

google::api::Metric ToMetric(
opentelemetry::sdk::metrics::MetricData const& metric_data,
opentelemetry::sdk::metrics::PointAttributes const& attributes,
Expand Down Expand Up @@ -89,6 +102,21 @@ std::vector<google::monitoring::v3::CreateTimeSeriesRequest> ToRequests(
std::string const& project, google::api::MonitoredResource const& mr_proto,
std::vector<google::monitoring::v3::TimeSeries> tss);

/**
* Copy some resource labels into metric labels.
*
* Some resource labels need to be copied into metric labels as they are not
* directly accepted by Google Cloud Monitoring as resource labels.
*
* For example, service resource labels need to be copied into metric labels.
* See:
* https://github.com/GoogleCloudPlatform/opentelemetry-operations-go/blob/main/exporter/collector/breaking-changes.md#labels
*/
void WithExtraLabels(opentelemetry::sdk::metrics::ResourceMetrics const& data,
std::vector<google::monitoring::v3::TimeSeries>& tss,
std::unordered_map<std::string, OTelKeyMatch> const&
extra_labels = kExtraLabelsLookup);

GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_END
} // namespace otel_internal
} // namespace cloud
Expand Down
90 changes: 90 additions & 0 deletions google/cloud/opentelemetry/internal/time_series_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,11 +20,13 @@
#include "google/cloud/testing_util/scoped_log.h"
#include <google/protobuf/text_format.h>
#include <gmock/gmock.h>
#include <opentelemetry/sdk/common/attribute_utils.h>
#include <opentelemetry/sdk/metrics/export/metric_producer.h>
#include <opentelemetry/sdk/resource/resource.h>
#include <opentelemetry/sdk/resource/semantic_conventions.h>
#include <algorithm>
#include <cstdint>
#include <string>

namespace google {
namespace cloud {
Expand Down Expand Up @@ -155,6 +157,18 @@ auto IsTestResource() {
Pair("instance_id", "1020304050607080900"))));
}

auto ResourceWithAllExtraLabels() {
opentelemetry::sdk::common::AttributeMap labels;
for (auto const& kv : kExtraLabelsLookup) {
auto const& oks = kv.second.otel_keys;
if (oks.empty()) {
continue;
}
labels.SetAttribute(oks[0], kv.first);
}
return opentelemetry::sdk::resource::Resource::Create(labels);
}

auto MetricType(std::string const& type) {
return ResultOf(
"metric.type",
Expand Down Expand Up @@ -697,6 +711,82 @@ TEST(ToRequests, Batches) {
}
}

TEST(WithExtraLabels, CopyLabels) {
opentelemetry::sdk::metrics::PointDataAttributes pda;
pda.point_data = opentelemetry::sdk::metrics::SumPointData{};

opentelemetry::sdk::metrics::MetricData md;
md.point_data_attr_.push_back(std::move(pda));
md.instrument_descriptor.name_ = "metric-name";
md.instrument_descriptor.unit_ = "unit";
md.instrument_descriptor.type_ = {};
md.instrument_descriptor.value_type_ = {};

opentelemetry::sdk::metrics::ScopeMetrics sm;
sm.metric_data_.push_back(md);
sm.metric_data_.push_back(std::move(md));

opentelemetry::sdk::metrics::ResourceMetrics rm;
auto resource = ResourceWithAllExtraLabels();
rm.resource_ = &resource;
rm.scope_metric_data_.push_back(std::move(sm));

auto tss = ToTimeSeries(rm, PrefixWithWorkload);
for (auto const& kv : kExtraLabelsLookup) {
auto const& oks = kv.second.otel_keys;
if (oks.empty()) {
continue;
}
for (auto const& ts : tss) {
Comment on lines +735 to +740
Copy link
Member

@dbolduc dbolduc Jan 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

auto const& labels = ts.metric().labels();
ASSERT_TRUE(labels.find(kv.first) != labels.end());
EXPECT_EQ(labels.at(kv.first), kv.first);
Comment on lines +741 to +743
Copy link
Member

@dbolduc dbolduc Jan 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We probably want to use GMock's container matchers.

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

}
}
}

TEST(WithExtraLabels, NoOverwrites) {
opentelemetry::sdk::metrics::PointDataAttributes pda;
opentelemetry::sdk::common::OrderedAttributeMap existing_labels;
existing_labels.SetAttribute("service_name", "do_not_overwrite");
pda.attributes = existing_labels;
pda.point_data = opentelemetry::sdk::metrics::SumPointData{};

opentelemetry::sdk::metrics::MetricData md;
md.point_data_attr_.push_back(std::move(pda));
md.instrument_descriptor.name_ = "metric-name";
md.instrument_descriptor.unit_ = "unit";
md.instrument_descriptor.type_ = {};
md.instrument_descriptor.value_type_ = {};

opentelemetry::sdk::metrics::ScopeMetrics sm;
sm.metric_data_.push_back(md);
sm.metric_data_.push_back(std::move(md));

opentelemetry::sdk::metrics::ResourceMetrics rm;
auto resource = ResourceWithAllExtraLabels();
rm.resource_ = &resource;
rm.scope_metric_data_.push_back(std::move(sm));

auto tss = ToTimeSeries(rm, PrefixWithWorkload);
for (auto const& kv : kExtraLabelsLookup) {
auto const& oks = kv.second.otel_keys;
if (oks.empty()) {
continue;
}
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) ==
Copy link
Member

@dbolduc dbolduc Jan 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ifs in tests kind of scare me.

existing_labels.GetAttributes().end()) {
EXPECT_EQ(labels.at(kv.first), kv.first);
} else {
EXPECT_EQ(labels.at(kv.first), "do_not_overwrite");
}
}
}
}

} // namespace
GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_END
} // namespace otel_internal
Expand Down