GoogleCloudPlatform / opentelemetry-operations-go

Apache License 2.0
130 stars 100 forks source link

Update usages of `faas.id` resource attribute to `faas.instance` #679

Closed damemi closed 1 year ago

damemi commented 1 year ago

Our resource detectors and monitored resource mappings currently rely on semconv v1.18, which has faas.id, which we use for the Generic Task (Cloud Run) resource.

As of semconv v1.19.0, faas.id is removed(/replaced with cloud.resource_id).

So, when we update to semconv v1.19, we'll need to update our usages of faas.id to faas.instance:

This attribute is also used in our code samples for GMP-Cloud Run integration: https://github.com/GoogleCloudPlatform/golang-samples/blob/e4eddad4457dc10f0fabb1b6f4c1906bea46aedc/run/custom-metrics/collector/collector-config.yaml#L45, which will need to be updated and the matching docs pages manually redeployed.

dashpole commented 1 year ago

Proposed Plan:

  1. Update GMP exporter to map faas.name to job, and faas.instance to instance.
  2. Update e2e test framework to expect the new attribute.
  3. Update Resource detectors (across languages) to detect faas.instance instead of faas.id, and update metric exporter mapping.
  4. Wait ~3 months to give time for collector releases to happen.
  5. Remove the resource processor from the sample.

For anyone using our current documentation, they would not be broken at any point.

@aabmass do you think that plan makes sense?

aabmass commented 1 year ago

SGTM. Do we consider this breaking for the genericTask mapping Mike mentioned? This would apply to users sending to Cloud Monitoring (not GMP). I think it should be ok

dashpole commented 1 year ago

It is breaking unless they are using our resource detector, which seems OK. We should recommend people use our resource detectors wherever possible.

dashpole commented 1 year ago

I need to update contrib for golang before I can update the this repo

dashpole commented 1 year ago

The collector's resource detector needs to be updated. I had forgotten about that.

aabmass commented 1 year ago

As of semconv v1.19.0, faas.id is removed(/replaced with cloud.resource_id).

So, when we update to semconv v1.19, we'll need to update our usages of faas.id to faas.instance

Sorry it's been a while and I'm confused again. If faas.id was renamed to cloud.resource_id, shouldn't we be using that? Or were we incorrectly using faas.id in the first place?

dashpole commented 1 year ago

We were incorrectly using faas.id in the first place. faas.id was meant to be the full resource name (e.g. projects/my-project/locations/us-central1/functions/my-function)

dashpole commented 1 year ago

We can track documentation updates in https://github.com/GoogleCloudPlatform/opentelemetry-cloud-run/issues/6. All other work is done