fix(otel): Exporter creating Monitored Resource with task_id for Cloud Run #14923
Conversation
DouglasHeriot
changed the title
DouglasHeriot
force-pushed
the
cloudrun
branch
from
1c0ab86 to
7a8688d
Compare
Fixes googleapis#14925 When inside a Cloud Run environment, the `MonitoredResource` in a `CreateTimeSeriesRequest` to the Cloud Monitoring API does not include the necessary fields for the `generic_task` resource type, and is rejected. Should follow the well-tested Golang implementation where the `faas.instance` OTel Resource Attribute is mapped to `MonitoredResource` `task_id`. As the `service.namespace` OTel Resource Attribute is not set by the Resource Detector from within Cloud Run, it should be mapped as an empty string, rather than being left absent. https://github.com/GoogleCloudPlatform/opentelemetry-operations-go/blob/8da0f42dab085c916987891419461d583a2aa96e/internal/resourcemapping/resourcemapping.go#L153
DouglasHeriot
force-pushed
the
cloudrun
branch
from
7a8688d to
644efe9
Compare
|
/gcbrun |
DouglasHeriot
temporarily deployed
to
external
GitHub Actions
Inactive
There was a problem hiding this comment.
Thanks for the PR!
The code looks good, although I offered an optional refactor. Unit tests for the new code paths would be nice. Either way, we need to fix the tests that are now broken.
You can run the unit test with:
bazelisk test --test_output=all //google/cloud/opentelemetry:internal_monitored_resource_test
| } else if (kv.second.fallback) { | ||
| mr.labels[kv.first] = *kv.second.fallback; | ||
| } else { | ||
| mr.labels[kv.first] = ""; |
There was a problem hiding this comment.
So it seems like we always fallback. Sometimes to a value like "global", but at least to "".
optional nit: It seems slightly cleaner to make fallback a std::string:
and then have this code just be:
} else {
mr.labels[kv.first] = kv.second.fallback;
}(If this doesn't work for whatever reason, just let me know, and I can look into cleaning it up in a future PR)
There was a problem hiding this comment.
That does seem cleaner, I’ll change the fallback to default to empty string.
| @@ -174,7 +176,7 @@ MonitoredResourceProvider GenericNode() { | |||
| {"location", | |||
| {{sc::kCloudAvailabilityZone, sc::kCloudRegion}, "global"}}, | |||
| {"namespace", {{sc::kServiceNamespace}}}, | |||
| {"node_id", {{sc::kHostId}}}, | |||
| {"node_id", {{sc::kHostId, sc::kHostName}}}, | |||
There was a problem hiding this comment.
Good catch, thanks!
Can we have a unit test for this?
I would probably change TestCase to something like LocationTestCase, and then introduce a NodeIdTestCase, and loop over:
for (auto l : location_tests) {
for (auto n : node_tests) {
// verify l.expected_location and n.expected_node_id
}
}| {"job", {{sc::kServiceName, sc::kFaasName}}}, | ||
| {"task_id", {{sc::kServiceInstanceId, sc::kFaasInstance}}}, |
There was a problem hiding this comment.
Again, good catch and thanks.
Can we have unit tests for these too? Or at least fix the broken tests.
Also, consider leaving a comment in a test case where we fallback to the empty string. Just something that says "Verify that we fallback to the empty string if no matches are found".
There was a problem hiding this comment.
Yep, I’ll add some tests for these cases.
Fixes #14925
When inside a Cloud Run environment, the
MonitoredResourcein aCreateTimeSeriesRequestto the Cloud Monitoring API does not include the necessary fields for thegeneric_taskresource type, and is rejected.Also updating the GenericNode mapping to match Golang.
This change is