fix(otel): Exporter creating Monitored Resource with task_id for Cloud Run #14923
Conversation
DouglasHeriot
changed the title
DouglasHeriot
force-pushed
the
cloudrun
branch
2 times, most recently
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
DouglasHeriot
force-pushed
the
cloudrun
branch
from
644efe9 to
7a0f137
Compare
|
/gcbrun |
| @@ -52,7 +52,7 @@ struct AsStringVisitor { | |||
|
|
|||
| struct OTelKeyMatch { | |||
| std::vector<std::string> otel_keys; | |||
| absl::optional<std::string> fallback = absl::nullopt; | |||
| std::string fallback; | |||
There was a problem hiding this comment.
might need to explicitly say std::string fallback = {} for our initializer lists to work
There was a problem hiding this comment.
I think this works already – std::string default constructs to empty string and the compiler doesn’t complain?
There was a problem hiding this comment.
So I am not smart enough to tell you why the compiler is complaining. (and only in cmake-based builds 🤷). But it is complaining:
e.g. this build: https://github.com/googleapis/google-cloud-cpp/pull/14923/checks?check_run_id=35680003918
There was a problem hiding this comment.
I think the simplest build to test on is:
ci/cloudbuild/build.sh -t demo-fedora-pr
There was a problem hiding this comment.
It looks like we had to do the same thing (explicitly initialize the field to its default value) when the type was absl::optional<std::string>. 🤷
There was a problem hiding this comment.
Ah you’re right. Fixed now. Thanks for the build command.
There was a problem hiding this comment.
Haha, clang-tidy doesn’t like this:
error: initializer for member 'fallback' is redundant [readability-redundant-member-init,-warnings-as-errors]
Step #2: 55 | std::string fallback = {};
I guess I’ll have to make it ignore this warning? Probably a better solution than ignoring the above -Wmissing-field-initializers warnings. // NOLINT(readability-redundant-member-init)
DouglasHeriot
force-pushed
the
cloudrun
branch
from
7a0f137 to
4c8e612
Compare
|
Sorry about the formatting, I think it’s fixed now. |
|
/gcbrun |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #14923 +/- ##
==========================================
- Coverage 92.92% 92.91% -0.01%
==========================================
Files 2351 2351
Lines 210072 210072
==========================================
- Hits 195204 195197 -7
- Misses 14868 14875 +7 ☔ View full report in Codecov by Sentry. |
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
4c8e612 to
579398c
Compare
|
/gcbrun |
|
/gcbrun |
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