-
Notifications
You must be signed in to change notification settings - Fork 103
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
feat: WithMonitoredResourceAttributes and WithMonitoredResourceDescription #850
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #850 +/- ##
==========================================
+ Coverage 61.03% 62.52% +1.48%
==========================================
Files 56 57 +1
Lines 5903 4931 -972
==========================================
- Hits 3603 3083 -520
+ Misses 2143 1688 -455
- Partials 157 160 +3 ☔ View full report in Codecov by Sentry. |
exporter/metric/option.go
Outdated
|
||
// metricAttributesFilter determinies which metric attributes to | ||
// add to metrics as metric labels | ||
metricAttributesFilter attribute.Filter |
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.
You shouldn't need this config in the exporter. Use https://pkg.go.dev/go.opentelemetry.io/otel/sdk/metric#WithView to pass views to the metrics SDK, which allow filtering attributes on metrics.
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.
with views, the metric attributes do not reach the exporter at all. The attributes need to reach the exporter. Then, the exporter can add them to resource
cc @psx95 |
This looks similar to the recent request we had for the Java exporter, I will take a look into this PR. |
A similar change to the Java exporter was just approved and merged in GoogleCloudPlatform/opentelemetry-operations-java#346. We would likely want to follow similar logic here. |
Similar functionality has been added in #854. |
Context: googleapis/google-cloud-go#10046
I'm working on exporting Bigtable client metrics to Google Cloud Monitoring.
I have tried using the exporter in its current form:
In step 15 in the above diagram, the monitored resource in CreateServiceTimeSeries request has 2 fields:
Problem 1: The CBT client side metrics need to be written to ‘bigtable_client_raw’ monitored resource type but the exporter does not allow writing to resource types other than the ones mentioned here
Problem 2: Only project and instance values are known at step 5 point in time. Table is not known until data operation such as ReadRows is invoked.
A Bigtable instance is a container for user's data. Instances have one or more clusters, located in different zones. When library sends requests to a Bigtable instance, those requests are handled by one of the clusters in the instance. The cluster is not known while sending the request. Cluster and zone need to be read from response headers of data operation. Thus, the existing metric exporter cannot add these labels to the monitored resource.
To resolve problem 1, solution similar to GoogleCloudPlatform/opentelemetry-operations-java#346 is being included in this PR.
How to include monitored resource labels?
WithMonitoredResourceAttributes
to extract the resource labels from the metric labels and add them to monitored resource. Example usage:https://github.com/bhshkh/google-cloud-go/blob/bc1d79ae1a71a774b96ab94b875b8a29c0046501/bigtable/metric_monitoring_exporter.go#L72-L75