Skip to content

Remove MR mapping for cloud run and cloud functions - #250

Merged
psx95 merged 10 commits into
GoogleCloudPlatform:mainfrom
psx95:cloud-run-service
Jun 13, 2023
Merged

psx95 merged 10 commits into
GoogleCloudPlatform:mainfrom
psx95:cloud-run-service

Conversation

@psx95

@psx95 psx95 commented May 18, 2023 •

Copy link
Copy Markdown
Contributor

Fix #248

This PR removes the explicit mapping to following Monitored Resource Types:

  • cloud_run_revision
  • cloud_function

These mappings are removed since these MR types do not support user-defined metrics. By removing their mappings, the exporter will by default map them to generic_task.

The PR also adds a convenience script that allows autoinstrument example to be able to run in cloud-run environment.

This script was used to verify that metrics can be sent from a cloud-run environment and they appear in Cloud Monitoring under Generic Task

Similar to work already done in - GoogleCloudPlatform/opentelemetry-operations-go#570

@psx95
psx95 requested review from jsuereth and punya May 18, 2023 01:25
@psx95
psx95 force-pushed the cloud-run-service branch from bf1e66c to 6e9e41e Compare May 18, 2023 21:08
psx95 added 5 commits June 8, 2023 21:34
…latform#249)

* Add readme with steps to run the example

* Add script to run example as Cloud Run Job

* Replace GCR with Artifact Registry

* Rename GOOGLE_CLOUD_RUN_JOB_REGION to GOOGLE_CLOUD_RUN_REGION
As per the exporter spec, we do not support pushing metric to cloud-run.
This change will cause cloud_run_revision to be treated as a
generic_task.
Cloud functions is also not writable for user-defined metrics, therefore
removing its explicit mapping so that it defaults to generic_task.
@psx95 psx95 changed the title WIP: Support metric exporter for cloud run Remove MR mapping for cloud run and cloud functions Jun 8, 2023
@psx95
psx95 marked this pull request as ready for review June 8, 2023 22:54
@psx95
psx95 requested a review from a team as a code owner June 8, 2023 22:54
@psx95 psx95 added the automerge Summon MOG for automerging label Jun 8, 2023
@gcf-merge-on-green

Copy link
Copy Markdown

Merge-on-green attempted to merge your PR for 6 hours, but it was not mergeable because either one of your required status checks failed, one of your required reviews was not approved, or there is a do not merge label. Learn more about your required status checks here: https://help.github.com/en/github/administering-a-repository/enabling-required-status-checks. You can remove and reapply the label to re-run the bot.

@gcf-merge-on-green gcf-merge-on-green Bot removed the automerge Summon MOG for automerging label Jun 9, 2023

@punya punya left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we implement a Cloud Logging exporter, we'd want to use cloud_run_revision rather than generic_node. That makes me wonder if our resource detection logic should take a signal type parameter in addition to an OTel resource so that we can take it into account.

curl -H "traceparent: 00-ff000000000000000000000000000041-ff00000000000041-01" ${cluster_ip}
```

## Running in Google Cloud Run

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In future, please keep unrelated changes (such as examples) in separate PRs.

Comment thread examples/autoinstrument/README.md
Comment thread examples/autoinstrument/run_in_cloud-run.sh
Comment thread examples/autoinstrument/run_in_cloud-run.sh Outdated
Comment on lines +71 to +76
This will deploy the containerized application to Cloud Run and you will be presented with a service URL which would look something like -

```text
Service URL: https://hello-autoinstrument-cloud-run-m43qtxry5q-uc.a.run.app
```

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that we're using the proxy, the service URL should no longer matter right?

@psx95 psx95 Jun 13, 2023 •

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this is just to clarify what to expect in case of a successful deployment. Also, I believe now we are adding the cURL instructions back, so this would be useful there.

Comment thread examples/autoinstrument/README.md
@psx95
psx95 merged commit e6c6a2b into GoogleCloudPlatform:main Jun 13, 2023
@psx95
psx95 deleted the cloud-run-service branch June 13, 2023 18:43
@Eden90

Eden90 commented Jun 28, 2023

Copy link
Copy Markdown

When is planned release with these changes?

aabmass added a commit to aabmass/opentelemetry-operations-js that referenced this pull request Oct 5, 2023
…races

In the trace exporter, stop passing `includeUnsupportedResources` param
(false by default). This param controls if the mapping function should
return `cloud_run_revision` and `cloud_function` monitored resources.
This is equivalent to what we do in [Go](https://github.com/GoogleCloudPlatform/opentelemetry-operations-go/blob/v0.44.0/internal/resourcemapping/resourcemapping.go) and Java GoogleCloudPlatform/opentelemetry-operations-java#250

Deprecate the `includeUnsupportedResources` parameter by splitting the
function def into a deprecated and non-deprecated overload. It is no
longer needed but the package is marked stable so I will save the
breaking change.
aabmass added a commit to GoogleCloudPlatform/opentelemetry-operations-js that referenced this pull request Oct 5, 2023
…races (#635)

In the trace exporter, stop passing `includeUnsupportedResources` param
(false by default). This param controls if the mapping function should
return `cloud_run_revision` and `cloud_function` monitored resources.
This is equivalent to what we do in [Go](https://github.com/GoogleCloudPlatform/opentelemetry-operations-go/blob/v0.44.0/internal/resourcemapping/resourcemapping.go) and Java GoogleCloudPlatform/opentelemetry-operations-java#250

Deprecate the `includeUnsupportedResources` parameter by splitting the
function def into a deprecated and non-deprecated overload. It is no
longer needed but the package is marked stable so I will save the
breaking change.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Metrics exporter does not support Cloud Run

3 participants