WIP: feat: Add Stackdriver integration #59

grant · 2019-07-19T19:33:08Z

WIP: Adds StackDriver Debug, Trace, and Profiler support.

TODO:

Add an env var option for disabling the integration

Fix the tests

Add setup to Travis: https://www.npmjs.com/package/@google-cloud/profiler#basic-set-up
- Create GOOGLE_APPLICATION_CREDENTIALS

Error: @google-cloud/profiler Failed to create profile

request to HTTP function

@google-cloud/profiler Failed to create profile, waiting 9.3s to try again: Error: Could not refresh access token: A Not Found error was returned while attempting to retrieve an accesstoken for the Compute Engine built-in service account. This may be because the Compute Engine instance does not have any permission scopes specified: Could not refresh access token: Unsuccessful response status code. Request failed with status code 404
@google-cloud/debug-agent Failed to re-register debuggee eco-emissary-99515: Error: Could not refresh access token: A Not Found error was returned while attempting to retrieve an accesstoken for the Compute Engine built-in service account. This may be because the Compute Engine instance does not have any permission scopes specified: Could not refresh access token: Unsuccessful response status code. Request failed with status code 404
1) should return transformed body

Open Questions

Should these be normal dependancies rather than devDependancies?
How can we test this PR?

Fixes: #57

package.json

squee1945 · 2019-07-19T19:38:38Z

src/tools.ts

+  }
+
+  // Start Trace
+  trace.start();


I thought the trace-agent needed to be started as the very first part of the app:

require('@google-cloud/trace-agent').start();

See https://www.npmjs.com/package/@google-cloud/trace-agent#usage

I don't think that's a hard requirement, we have it imported after http and on-finish modules, which I don't think have a starter script like this.

I think that suggestion is just added to ensure that the full trace is added rather than a partial trace.

squee1945 · 2019-07-19T19:40:39Z

src/invoker.ts

-    d.run(() => {
+    d.run(async () => {
+      // Start the debugger (if not alreay started)
+      const tools = startTooling();


There is overhead both in starting the tooling, and ongoing performance overhead. Is there a way for the user to opt out of this?

For ease in turning instrumentation on and off, it would be nice to toggle this with an environment variable instead of something requiring a redeployment (e.g., adjusted container entrypoint)

Yes. The PRD specifies the following semantics:

Enable/disable Stackdriver APM
As a developer, I can enable or disable individual Stackdriver APM features. The on/off state is controlled by an environment variable as follows:

Stackdriver APM feature Environment variable

Stackdriver Trace GOOGLE_CLOUD_TRACE_ENABLED

If the environment variable is set to false or if it is not set, the feature should be disabled by default.

By default, Stackdriver APM features are enabled on Cloud Functions
As a developer on Cloud Functions, when I deploy my function Stackdriver Trace is enabled by default. The container into which my function is deployed will, e.g., automatically set the value of the GOOGLE_CLOUD_TRACE_ENABLED environment variable to true.

squee1945 · 2019-07-19T19:43:13Z

src/invoker.ts

@@ -26,6 +26,8 @@ import * as express from 'express';
 import * as http from 'http';
 import * as onFinished from 'on-finished';

+import { startTooling } from './tools';


Nit: diagnostics instead of tools?

I am not sure if the specific file name matters.

grayside · 2019-07-19T19:57:20Z

src/tools.ts

@@ -0,0 +1,60 @@
+import * as debugAgent from '@google-cloud/debug-agent';
+import * as profiler from '@google-cloud/profiler';


I understand that profiler currently has limitations on working with musl.

OK. Do you recommend removing it? Or what is the suggestion?

grant · 2019-08-26T19:52:50Z

In terms of fixing this PR, we need to setup gcloud with Travis and add the env var.
Added these TODOs to the PR description.

grant · 2020-05-18T21:54:06Z

This PR needs more of a design doc and solution across all FFs. Let's continue in the GitHub issue.

feat: Add Stackdriver integration

b273e1f

googlebot added the cla: yes label Jul 19, 2019

squee1945 reviewed Jul 19, 2019

View reviewed changes

package.json Show resolved Hide resolved

squee1945 reviewed Jul 19, 2019

View reviewed changes

grayside reviewed Jul 19, 2019

View reviewed changes

grant closed this May 18, 2020

grant deleted the grant_stackdriver_trace branch March 23, 2021 15:59

grant restored the grant_stackdriver_trace branch March 23, 2021 15:59

grant deleted the grant_stackdriver_trace branch March 23, 2021 16:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: feat: Add Stackdriver integration #59

WIP: feat: Add Stackdriver integration #59

grant commented Jul 19, 2019 •

edited

Loading

squee1945 Jul 19, 2019

grant Aug 26, 2019

squee1945 Jul 19, 2019

grayside Jul 19, 2019

stew-r Jul 19, 2019

squee1945 Jul 19, 2019

grant Aug 26, 2019

grayside Jul 19, 2019

grant Aug 26, 2019

grant commented Aug 26, 2019

grant commented May 18, 2020

		@@ -0,0 +1,60 @@
		import * as debugAgent from '@google-cloud/debug-agent';
		import * as profiler from '@google-cloud/profiler';

WIP: feat: Add Stackdriver integration #59

WIP: feat: Add Stackdriver integration #59

Conversation

grant commented Jul 19, 2019 • edited Loading

TODO:

Open Questions

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grant commented Aug 26, 2019

grant commented May 18, 2020

grant commented Jul 19, 2019 •

edited

Loading