fix(storage): monitored resource detection#11197
Conversation
|
Spot checked resulting monitored resource values to double check: |
tritone
left a comment
There was a problem hiding this comment.
Few minor questions/nits, otherwise looks good.
|
|
||
| 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?
| 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?
| { | ||
| 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.
| "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.
Summary of changes
_instead of., e.g. "host_id" instead of "host.id" resulting in storageClient always using default attribute values.opentelemetry.DefaultMetrics()