Skip to content

feat: resource autodetection for cloud run#1024

Merged
freelerobot merged 7 commits intomasterfrom
resourceDetection
Apr 5, 2021
Merged

feat: resource autodetection for cloud run#1024
freelerobot merged 7 commits intomasterfrom
resourceDetection

Conversation

@freelerobot
Copy link
Contributor

@freelerobot freelerobot commented Apr 2, 2021 •
edited
Loading

Fixes #959 🦕

Changes:

  • User logs to CloudRun automatically contain resource labels per contract
  • Unit tests pass
  • End-to-end environment tests pass with correct resource types/labels. To repro: run nox --python 3.7 --session "tests(language='nodejs', platform=‘cloudrun’)” in /env-tests-logging

Get

nox > Running session tests(language='nodejs', platform='cloudrun')
nox > Creating virtual environment (virtualenv) using python3.7 in .nox/tests-language-nodejs-platform-cloudrun
nox > python -m pip install --pre grpcio
nox > python -m pip install mock pytest google-cloud-testutils google-cloud-bigquery google-cloud-pubsub google-cloud-storage google-cloud-testutils google-cloud-logging
nox > py.test -s ./tests/nodejs/test_cloudrun.py
======================================================== test session starts ========================================================
platform darwin -- Python 3.7.9, pytest-6.2.2, py-1.10.0, pluggy-0.13.1
rootdir: /Users/nicolezhu/Desktop/nodejs-logging/env-tests-logging
collected 3 items

tests/nodejs/test_cloudrun.py ['/Users/nicolezhu/Desktop/nodejs-logging/env-tests-logging/envctl/envctl', 'nodejs', 'cloudrun', 'verify']
['/Users/nicolezhu/Desktop/nodejs-logging/env-tests-logging/envctl/envctl', 'nodejs', 'cloudrun', 'deploy']
['/Users/nicolezhu/Desktop/nodejs-logging/env-tests-logging/envctl/envctl', 'nodejs', 'cloudrun', 'verify']
['/Users/nicolezhu/Desktop/nodejs-logging/env-tests-logging/envctl/envctl', 'nodejs', 'cloudrun', 'trigger', 'simplelog', 'log_text="test_monitored_resource 5e611698-9406-11eb-82d3-3e22fb28907a"']
, Trying again in 2 seconds...
['/Users/nicolezhu/Desktop/nodejs-logging/env-tests-logging/envctl/envctl', 'nodejs', 'cloudrun', 'trigger', 'simplelog', 'log_text="test_monitored_resource 89883162-9406-11eb-82d3-3e22fb28907a"']
.['/Users/nicolezhu/Desktop/nodejs-logging/env-tests-logging/envctl/envctl', 'nodejs', 'cloudrun', 'trigger', 'simplelog', 'log_text="test_receive_log 939056b2-9406-11eb-82d3-3e22fb28907a"']
..

=================================================== 3 passed in 172.87s (0:02:52) ===================================================
nox > Session tests(language='nodejs', platform='cloudrun') was successful.

Before:
image

After:
Screen Shot 2021-04-02 at 3 46 03 PM

@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Apr 2, 2021
@product-auto-label product-auto-label bot added the api: logging Issues related to the googleapis/nodejs-logging API. label Apr 2, 2021
@freelerobot freelerobot self-assigned this Apr 2, 2021
@freelerobot freelerobot added the priority: p2 Moderately-important priority. Fix may not be included in next release. label Apr 2, 2021
@codecov
Copy link

codecov bot commented Apr 2, 2021 •
edited
Loading

Codecov Report

Merging #1024 (b228d58) into master (f32da13) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1024   +/-   ##
=======================================
  Coverage   97.91%   97.92%           
=======================================
  Files          18       18           
  Lines       13160    13190   +30     
  Branches      399      421   +22     
=======================================
+ Hits        12886    12916   +30     
  Misses        271      271           
  Partials        3        3
Impacted Files Coverage Δ
src/metadata.ts 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f32da13...b228d58. Read the comment docs.

@freelerobot freelerobot marked this pull request as ready for review April 2, 2021 22:57
@freelerobot freelerobot requested a review from a team April 2, 2021 22:57
@freelerobot freelerobot requested a review from a team as a code owner April 2, 2021 22:57
simonz130
simonz130 reviewed Apr 5, 2021
View reviewed changes
src/metadata.ts
// everything runs on top of compute engine.
return getGCEDescriptor().catch(() => getGlobalDescriptor());
// Google Cloud Run
if (process.env.K_CONFIGURATION) {
Copy link

@simonz130 simonz130 Apr 5, 2021

Choose a reason for hiding this comment

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

Is there a reason it doesn't follow a similar pattern as the rest of them? GCPEnv.CLOUD_RUN?

Copy link
Contributor Author

@freelerobot freelerobot Apr 5, 2021

Choose a reason for hiding this comment

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

GCPEnv from google-auth-library detects cloudrun as compute_engine. So I look for the K_CONFIG envvar unique to cloud run within this switch case

Also I just filed issue for this in google-auth-lib here: googleapis/google-auth-library-nodejs#1155

test/metadata.ts
Comment on lines +157 to +159
const K_SERVICE = 'hello-world';
const K_REVISION = 'hello-world.1';
const K_CONFIGURATION = 'hello-world';
Copy link

@simonz130 simonz130 Apr 5, 2021

Choose a reason for hiding this comment

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

let's make the values similar to actual ones.

Copy link
Contributor Author

@freelerobot freelerobot Apr 5, 2021

Choose a reason for hiding this comment

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

I followed the official documentation on the sample values. https://cloud.google.com/run/docs/reference/container-contract

simonz130
simonz130 approved these changes Apr 5, 2021
View reviewed changes
Copy link

@simonz130 simonz130 left a comment

Choose a reason for hiding this comment

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

Great work!

@freelerobot freelerobot merged commit 08c4404 into master Apr 5, 2021
@freelerobot freelerobot deleted the resourceDetection branch April 5, 2021 22:48
@freelerobot freelerobot mentioned this pull request Apr 5, 2021
Closed
@Daniel-Aaron-Bloom Daniel-Aaron-Bloom mentioned this pull request Aug 20, 2025
Open
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@simonz130 simonz130 simonz130 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@freelerobot freelerobot

Labels

api: logging Issues related to the googleapis/nodejs-logging API. cla: yes This human has signed the Contributor License Agreement. priority: p2 Moderately-important priority. Fix may not be included in next release.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Auto-detection when used on Cloud Run (managed)

2 participants

@freelerobot @simonz130