Skip to content
This repository has been archived by the owner on Jun 5, 2023. It is now read-only.

Add OpenCensus instrumentation for tracing #1926

Merged
merged 45 commits into from
Oct 31, 2018

Conversation

ocervell
Copy link
Contributor

@ocervell ocervell commented Aug 21, 2018

This PR brings support for OpenCensus tracing for all gRPC Forseti services:

  • Add OpenCensus interceptors to Forseti's gRPC server and client
  • Set up an exporter of traces to Stackdriver Trace (plan on supporting more via cfg changes)
  • Add appropriate IAM role cloudtrace.agent to allow client/server VMs to write traces to Stackdriver (during deployment)
  • Add 'opencensus' and 'google-cloud-trace' dependencies

@ocervell
Copy link
Contributor Author

ocervell commented Aug 21, 2018

Seems like the library fails to contact Stackdriver API on the Travis CI test VM, do we have gcloud credentials set up on that VM ?

Maybe we can just disable the StackdriverExporter in testing mode and have the FileExporter that works locally as a default instead.

@blueandgold
Copy link
Contributor

Relates to #1845, #1438

Olivier Cervello and others added 4 commits August 22, 2018 21:53
* Shorten import statements
* Fix > 80 characters lines
* Fix unittests by falling back on FileExporter
@googlebot
Copy link

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

@codecov
Copy link

codecov bot commented Aug 24, 2018

Codecov Report

Merging #1926 into dev will decrease coverage by 0.03%.
The diff coverage is 70.37%.

@@            Coverage Diff             @@
##              dev    #1926      +/-   ##
==========================================
- Coverage   89.32%   89.28%   -0.04%     
==========================================
  Files         165      166       +1     
  Lines       12154    12180      +26     
==========================================
+ Hits        10856    10875      +19     
- Misses       1298     1305       +7
Impacted Files Coverage Δ
google/cloud/forseti/services/client.py 77.94% <100%> (+0.22%) ⬆️
google/cloud/forseti/services/server.py 64.78% <50%> (+0.5%) ⬆️
google/cloud/forseti/services/tracing.py 69.56% <69.56%> (ø)
.../cloud/forseti/notifier/notifiers/cscc_notifier.py 59.32% <0%> (ø) ⬆️
...cloud/forseti/scanner/scanners/bigquery_scanner.py 80% <0%> (ø) ⬆️

@ocervell
Copy link
Contributor Author

ocervell commented Aug 24, 2018

I signed it ! I have configured the right email address for future.

@ahoying
Copy link
Collaborator

ahoying commented Aug 24, 2018

Please make this change aware of if the opencensus package is installed, if it is not installed it should be a no-op.

For changes like this, where a new third party library is introduced for an optional feature, or a feature that can be disabled without impacting the rest of forseti, the code needs to fail back cleanly when that library is not installed.

This happens to be the first new feature to be implemented where this would apply.

@ocervell
Copy link
Contributor Author

We're planning on using OpenCensus for our monitoring as well.

While tracing could be made optional and configurable, monitoring (exposing OpenCensus metrics to monitoring systems like Stackdriver) seems required for Forseti, so the dependency would be as well.

Is there currently any way to add an optional dependency in Forseti deployments ?

@mirons-google
Copy link
Contributor

Since this is a first step in working to solve #1845 #1844 and #1438 I think it makes sense to consider the package as required.

The preferred way to do that is in setup.py currently.

@ahoying
Copy link
Collaborator

ahoying commented Aug 24, 2018

Forseti is both a deployment but also a set of libraries that are used elsewhere. Introducing additional third party dependencies makes the use of forseti libraries more difficult when a full deployment is not desired.

I recommend wrapping the opencensus imports in the tracing module in a try catch, and setting a global to True or False depending on if they are available or not.

Then the code can handle the cases where opencensus is installed or not installed by checking the value of that global, and you can fall back gracefully.

Olivier Cervello added 3 commits August 24, 2018 20:01
…n / off (#2)

* Add support for additional interceptors.

* Add server CLI flag `--enable-tracing`.

* Make tracing libraries optional. Install them using `pip install .[tracing]`.

* Pass `--enable-tracing` in `systemctl` script (default toggle for deployments)

* Add logging to exporter and tracer
@ocervell
Copy link
Contributor Author

ocervell commented Aug 27, 2018

@ahoying made some good points here. Users should be able to install the forseti-security library without extra dependencies.

My last commit brings support for:

  • More interceptors to be added (e.g a MetricsInterceptor)
  • A server CLI flag --enable-tracing that's disabled by default.
  • Added --enable-tracing to our systemctl startup script command.
  • Tracing libraries removed as required dependencies and added as OPTIONAL_DEPENDENCIES to setup.py. They can be installed using pip install .[tracing], statement I've added to our Forseti deployments that will enable tracing by default.

Please review and let me know what you think.

Copy link
Collaborator

@ahoying ahoying left a comment

Choose a reason for hiding this comment

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

Thank you for taking the extra time on this one to make it optional. Looks great, I think this is a solid pattern for other features.

Copy link
Contributor

@blueandgold blueandgold left a comment

Choose a reason for hiding this comment

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

A couple of more comments.

google/cloud/forseti/services/client.py Outdated Show resolved Hide resolved
Olivier Cervello added 3 commits September 18, 2018 13:46
* Clean up tracing

* Fix logging error

* Better imports for OpenCensus

* Remove unused 'imp' import
@ocervell
Copy link
Contributor Author

ocervell commented Oct 4, 2018

Thanks for your in-depth review. I think I've addressed the above comments with my last commits.
The tests are failing for an un-related issue, should I merge the latest from dev ?

@blueandgold blueandgold changed the title [WIP] Add OpenCensus instrumentation for tracing Add OpenCensus instrumentation for tracing Oct 26, 2018
Copy link
Contributor

@blueandgold blueandgold left a comment

Choose a reason for hiding this comment

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

This looks really great! Thank you so much for the work here! We will test it during our release process next week!

Copy link
Contributor

@blueandgold blueandgold left a comment

Choose a reason for hiding this comment

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

Awesome, looking forward to having instrumentation!

@red2k18 red2k18 added cla: yes and removed cla: no labels Oct 31, 2018
@googlebot
Copy link

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

@red2k18 red2k18 merged commit 3ec2263 into forseti-security:dev Oct 31, 2018
blueandgold added a commit that referenced this pull request Nov 1, 2018
blueandgold added a commit that referenced this pull request Nov 1, 2018
* Revert "Fix applies_to in location scanner rule (#2161)"

This reverts commit ca65e41.

* Revert "Add OpenCensus instrumentation for tracing (#1926)"

This reverts commit 3ec2263.
@dekuhn dekuhn added this to In progress in Forseti Inventory Performance Aug 16, 2019
@dekuhn dekuhn moved this from In progress to Done in Forseti Inventory Performance Aug 16, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

7 participants