-
Notifications
You must be signed in to change notification settings - Fork 39
fix: support for Cloud Run monitored resource #78
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Codecov Report
@@ Coverage Diff @@
## master #78 +/- ##
============================================
- Coverage 76.24% 75.96% -0.29%
Complexity 571 571
============================================
Files 42 42
Lines 3334 3353 +19
Branches 232 237 +5
============================================
+ Hits 2542 2547 +5
- Misses 647 660 +13
- Partials 145 146 +1
Continue to review full report at Codecov.
|
@@ -40,10 +40,13 @@ | |||
ContainerName("container_name"), | |||
InstanceId("instance_id"), | |||
InstanceName("instance_name"), | |||
Location("location"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason to use "location" instead of "region"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I looked at log entries that get ingested from stdout, and they all have the location
attribute. So, I tried to match that.
@@ -171,6 +185,11 @@ private static String getValue(Label label) { | |||
|
|||
/* Detect monitored Resource type using environment variables, else return global as default. */ | |||
private static Resource getAutoDetectedResourceType() { | |||
if (System.getenv("K_SERVICE") != null && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Steren's note: Note that checking the presence of K_SERVICE and other env vars is not sufficient to differentiate between Cloud Run and any other Knative installation (e.g. Cloud Run for Anthos).
Would adding a check for "KUBERNETES_SERVICE_HOST" == null, fix this issue?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that makes sense.
@chingor13 can you approve? |
Fixes: #71.