Skip to content
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

Turn off GCP integrations by default #665

Merged
merged 4 commits into from
Dec 29, 2021
Merged

Conversation

askmeegs
Copy link
Contributor

Background

Turns off Google Cloud Monitoring, Tracing, Profiler, and Debugger for all services by default, to re-enable out of the box support for local clusters.

Fixes

#663

Change Summary

  1. Comments out DISABLE_PROFILER=1 etc for all services, thus turning off GCP integrations by default
  2. Payments service, for some reason, didn't look at these env variables so none of the GCP integraitons were toggle-able. fixed that.

Testing Procedure

  1. Create a local cluster (kind or minikube)
  2. Have a project ID ready to go, PROJECT_ID env field set (for GCR)
  3. skaffold run --default-repo=gcr.io/$PROJECT_ID/onlineboutique this will test the kubernetes-manifests, not release/ on a local cluster. that's what you want.
  4. Verify that all pods start up without errors.

@askmeegs askmeegs requested a review from a team December 16, 2021 18:58
@github-actions
Copy link

🚲 PR staged at http://34.66.119.59

Copy link
Contributor

@ahrarmonsur ahrarmonsur left a comment

Choose a reason for hiding this comment

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

Code looks good. I manually tested, following the guidelines, and was able to get the app stood up without errors. The logs posted the following, as the expected default behaviour:

Profiler disabled.
Tracing disabled.
Debugger disabled.

LGTM!

@askmeegs askmeegs changed the title Turn off GCP integrations by default [Do not merge] Turn off GCP integrations by default Dec 21, 2021
Copy link
Collaborator

@NimJay NimJay left a comment

Choose a reason for hiding this comment

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

Thanks for letting me review this before merging, @askmeegs!
I've left 2 comments.

src/paymentservice/index.js Show resolved Hide resolved
kubernetes-manifests/adservice.yaml Show resolved Hide resolved
@github-actions
Copy link

🚲 PR staged at http://34.66.119.59

@github-actions
Copy link

🚲 PR staged at http://34.66.119.59

1 similar comment
@github-actions
Copy link

🚲 PR staged at http://34.66.119.59

@askmeegs askmeegs changed the title [Do not merge] Turn off GCP integrations by default Turn off GCP integrations by default Dec 29, 2021
@NimJay NimJay self-requested a review December 29, 2021 16:59
Copy link
Collaborator

@NimJay NimJay left a comment

Choose a reason for hiding this comment

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

Thanks so much for doing this, @askmeegs!
Changes look good to me!
Ahrar has also done the appropriate testing.
Approved and merged.

Unrelated Note
I realized that the code style (e.g., bracket style) varies across this repository.
But again, this concern is definitely outside the scope of this pull-request.
Thus, I've created a separate GitHub issue about enforcing consistent coding style in JavaScript/Node.js service: #675.

@@ -0,0 +1,25 @@
# Integrating Online Boutique with Google Cloud
Copy link
Collaborator

Choose a reason for hiding this comment

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

Praise: Thanks for documenting this, Megan!

src/paymentservice/index.js Show resolved Hide resolved
@NimJay NimJay merged commit 2e796c9 into master Dec 29, 2021
@NimJay NimJay deleted the reenable-local-clusters branch December 29, 2021 17:28
sitaramkm added a commit to sitaramkm/microservices-demo that referenced this pull request Mar 27, 2022
* Turn off GCP integrations by default

* Add doc

* cleanup
copybaranaut pushed a commit to pixie-io/pixie that referenced this pull request Jan 12, 2023
Summary:
Fixes #689.

The `px-online-boutique` demo app attempts to connect to GCP integrations. This causes problems when deployed on non-GCP clusters. This diff turns off GCP integrations by default for the demo app. This change has already been addressed upstream in GCP's repo, but at this time we do not want to update our fork of the repo. Upstream fix: GoogleCloudPlatform/microservices-demo#665

Test Plan: Manually deploy modified yaml file on GCP and EKS cluster.

Reviewers: michelle

Reviewed By: michelle

Signed-off-by: Hannah Troisi <htroisi@pixielabs.ai>

Differential Revision: https://phab.corp.pixielabs.ai/D12773

GitOrigin-RevId: cc6645c78a19e33af4ee9bb70b4995ee1125850d
RagalahariP pushed a commit to RagalahariP/pixie that referenced this pull request Feb 15, 2023
Summary:
Fixes pixie-io#689.

The `px-online-boutique` demo app attempts to connect to GCP integrations. This causes problems when deployed on non-GCP clusters. This diff turns off GCP integrations by default for the demo app. This change has already been addressed upstream in GCP's repo, but at this time we do not want to update our fork of the repo. Upstream fix: GoogleCloudPlatform/microservices-demo#665

Test Plan: Manually deploy modified yaml file on GCP and EKS cluster.

Reviewers: michelle

Reviewed By: michelle

Signed-off-by: Hannah Troisi <htroisi@pixielabs.ai>

Differential Revision: https://phab.corp.pixielabs.ai/D12773

GitOrigin-RevId: cc6645c78a19e33af4ee9bb70b4995ee1125850d
D-Mwanth pushed a commit to D-Mwanth/microservices-demo that referenced this pull request Mar 6, 2024
* Turn off GCP integrations by default

* Add doc

* cleanup
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.

3 participants