fix(storage): monitored resource detection #11197
Conversation
product-auto-label
bot
added
the
api: storage
|
Spot checked resulting monitored resource values to double check: |
| @@ -118,6 +119,24 @@ type grpcStorageClient struct { | |||
| settings *settings | |||
| } | |||
|
|
|||
| func enableClientMetrics(ctx context.Context, s *settings, config storageConfig) (*metricsContext, error) { | |||
| var project string | |||
| // TODO: use new auth client | |||
There was a problem hiding this comment.
do we have a bug for this?
storage/grpc_metrics.go
Outdated
| mexporter.WithMonitoredResourceDescription(monitoredResourceName, []string{"project_id", "location", "cloud_platform", "host_id", "instance_id", "api"}), | ||
| ) | ||
| if err != nil { | ||
| return nil, err |
There was a problem hiding this comment.
Let's wrap this with a string like "storage: creating metrics exporter: %w"
| p, present := s.Value("cloud.account.id") | ||
| if present { | ||
| preparedResource.projectToUse = p.AsString() | ||
| if p, present := s.Value("cloud.account.id"); present && smr.project == "" { |
There was a problem hiding this comment.
Maybe add a comment about what this is trying to do?
storage/grpc_metrics_test.go
Outdated
| detectedAttributes []attribute.KeyValue | ||
| wantAttributes attribute.Set | ||
| }{ | ||
| { | ||
| desc: "default values set when GCP attributes are not detected", | ||
| desc: "default values set when GCP attributes are not detected", | ||
| project: "project-id", |
There was a problem hiding this comment.
project and api seem the same in all test cases, should they just be removed as fields here? Or did you want to test other values?
| }, withTestMetricReader(mr)) | ||
| } | ||
|
|
||
| func TestIntegration_MetricsEnablementInGCE(t *testing.T) { |
There was a problem hiding this comment.
Do we expect this test to run without skipping in Kokoro?
There was a problem hiding this comment.
That's correct. To test the listed metrics you need direct connectivity so it expects a GCE instance;
Is there incorrect logic?
There was a problem hiding this comment.
No, looks fine to me, just wanted to confirm.
storage/integration_test.go
Outdated
| "grpc.lb.rls.cache_entries": false, | ||
| "grpc.lb.rls.cache_size": false, | ||
| "grpc.lb.rls.default_target_picks": false, | ||
| // TODO: determine a way to force these metrics to be collected |
There was a problem hiding this comment.
Remove commented values here; please file an issue if there is other stuff we need to add later.
| }, withTestMetricReader(mr)) | ||
| } | ||
|
|
||
| func TestIntegration_MetricsEnablementInGCE(t *testing.T) { |
There was a problem hiding this comment.
No, looks fine to me, just wanted to confirm.
tritone
added
the
automerge
gcf-merge-on-green
bot
removed
the
automerge
Summary of changes
_instead of., e.g. "host_id" instead of "host.id" resulting in storageClient always using default attribute values.opentelemetry.DefaultMetrics()