Skip to content
This repository was archived by the owner on Apr 3, 2024. It is now read-only.

Add the start() Method and the Ability to Specify the Service Name/Version in the Debug Config#167

Merged
DominicKramer merged 18 commits intogoogleapis:masterfrom
DominicKramer:feature/extend-debug-config
Nov 1, 2016
Merged

Add the start() Method and the Ability to Specify the Service Name/Version in the Debug Config#167
DominicKramer merged 18 commits intogoogleapis:masterfrom
DominicKramer:feature/extend-debug-config

Conversation

@DominicKramer
Copy link
Copy Markdown
Contributor

This PR contains an update where the new start() method can be called on the object returned from require('@google/cloud-debug) to start the debugger while specifying a configuration.

Additionally, the configuration can now specify custom values for the app's service name and service version.

This PR replaces PR #166

Now the cloud debugger is started by explicitly calling the
newly exposed `start()` method that accepts an optional config
object.  Currently if the `start()` method is not called
within 5 seconds it is automatically called and the user is
issued a message detailing that the `start()` method will need
to be explicitly called in future releases.
In particular, the `GAE_MODULE_NAME` environment variable takes
priority over the `serviceName` config item and the
`GAE_MODULE_VERSION` environment variable takes priority over
the `serviceVersion` config item.
In particular, if the config object supplied to the `start()`
method has a `debug` attribute then the value of that attribute
will be used as the config.  Otherwise, as long as the config
object is defined, it is assumed to directly contain the
config options.
The values of `serviceName` and `serviceVersion` were previously
determined in `lib/debugletapi.js` by determining if certain
environment variables are set.  Now that check is done in
`index.js`.
The unit tests have been updated so that the `start()` method
is called when requiring `index.js`.
If the GCLOUD_DEBUG_LOGLEVEL environment variable is set then
its value will be used as the log level instead of the value
specified in the supplied configuration file.  However, the
unit test had the value of GCLOUD_DEBUG_LOGLEVEL and the value
in the config file both set to the same value.  Thus, the test
wouldn't verify if the environment variable was actually taking
precedence.
The tests verify that the value of the `GAE_MODULE_NAME`
environment variable (if defined) takes precendence over the
config value of `serviceName`.

Similar checks are also done for the environment variable
`GAE_MODULE_VERSION` and the config option `serviceVersion`.
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Oct 28, 2016
@DominicKramer
Copy link
Copy Markdown
Contributor Author

@GoogleCloudPlatform/node-team PTAL

This PR replaces PR #166, which I closed to avoid confusion.

The checking of whether the service version was 'default' was
incorrectly only checking the environment variable
GAE_MODULE_VERSION.
@jmdobry
Copy link
Copy Markdown
Contributor

jmdobry commented Oct 28, 2016

@GoogleCloudPlatform/node-team

Everybody on @GoogleCloudPlatform/node-team is watching the cloud-debug-nodejs and cloud-trace-nodejs repositories already, and therefore gets notifications. Is it okay with y'all if we cut back on using @GoogleCloudPlatform/node-team to explicitly re-notify everybody?

index.js Outdated
}
}

// exports is populated by the agent

This comment was marked as spam.

This comment was marked as spam.

index.js Outdated
log.error('The cloud-debug agent has been automatically started. ' +
'This action will be deprecated in the future.');
}
}, 5*1000);

This comment was marked as spam.

This comment was marked as spam.

The test in `test-module.js` that tests that two calls to
`require()` return the same object was updated to call the
`start()` method on both requires.
Copy link
Copy Markdown
Contributor

@matthewloring matthewloring left a comment

Choose a reason for hiding this comment

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

LGTM.

index.js Outdated
setTimeout(function() {
if (!started_){
start();
log_.error('The cloud-debug agent has been automatically started. ' +

This comment was marked as spam.

This comment was marked as spam.

Now the service name and version is specified as
```
   serviceContext: {
      service: 'service-name',
      version: '1'
   }
```
index.js Outdated
var config = initConfig();
module.exports = {
start: start,
hasStarted: hasStarted

This comment was marked as spam.

This comment was marked as spam.

That is, the config is not expected to contain a `debug`
attribute that contains the actual configuration options.
This change makes the debug agent consistent with the trace
agent.
In particular, tests were added to verify that the order of
priority from highest to lowest is
```
  env var -> config given to start() -> config file
```
@matthewloring
Copy link
Copy Markdown
Contributor

Could you update the title to reflect that this introduces the start function/configuration flow as well?

@DominicKramer DominicKramer changed the title Add the Ability to Specify the Service Name/Version in the Debug Config Add the start() Method and the Ability to Specify the Service Name/Version in the Debug Config Nov 1, 2016
@DominicKramer DominicKramer merged commit a131faf into googleapis:master Nov 1, 2016
@DominicKramer DominicKramer deleted the feature/extend-debug-config branch November 1, 2016 23:34
DominicKramer added a commit that referenced this pull request Nov 2, 2016
This PR adds a configuration section to the README file that should have been added in PR #167.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

cla: yes This human has signed the Contributor License Agreement.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants