Skip to content

Commit

Permalink
fix: update to @google-cloud/common@0.19 (#772)
Browse files Browse the repository at this point in the history 
* fix: update to @google-cloud/common@0.19

* chore: update package-lock.json

* test: update most of the tests that fail

* test: rewrite test-trace-writer

* refactor: address comments
  • Loading branch information
@kjin
kjin authored Jun 8, 2018
1 parent 7b9e48f commit 3f3f667
Show file tree
Hide file tree
Showing 18 changed files with 844 additions and 1,490 deletions.
1,068 changes: 337 additions & 731 deletions package-lock.json
View file

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion package.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@
"typescript": "~2.7.2"
},
"dependencies": {
"@google-cloud/common": "^0.17.0",
"@google-cloud/common": "^0.19.1",
"builtin-modules": "^2.0.0",
"continuation-local-storage": "^3.2.1",
"extend": "^3.0.0",
Expand Down
89 changes: 43 additions & 46 deletions src/trace-writer.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ headers[Constants.TRACE_AGENT_REQUEST_HEADER] = 1;
/* A list of scopes needed to operate with the trace API */
const SCOPES: string[] = ['https://www.googleapis.com/auth/trace.append'];

export interface TraceWriterConfig extends common.ServiceAuthenticationConfig {
export interface TraceWriterConfig extends common.GoogleAuthOptions {
projectId?: string;
onUncaughtException: string;
bufferSize: number;
Expand Down Expand Up @@ -82,7 +82,9 @@ export class TraceWriter extends common.Service {
config);

this.logger = logger;
this.config = config;
// Clone the config object
this.config = {...config};
this.config.serviceContext = {...this.config.serviceContext};
this.buffer = [];
this.defaultLabels = {};

Expand Down Expand Up @@ -121,22 +123,21 @@ export class TraceWriter extends common.Service {

// Schedule periodic flushing of the buffer, but only if we are able to get
// the project number (potentially from the network.)
this.getProjectId((err: Error|null, project?: string) => {
if (err) {
this.logger.error(
'TraceWriter#initialize: Unable to acquire the project number',
'automatically from the GCP metadata service. Please provide a',
'valid project ID as environmental variable GCLOUD_PROJECT, or as',
`config.projectId passed to start. Original error: ${err}`);
cb(err);
} else {
this.config.projectId = project;
this.scheduleFlush();
if (--pendingOperations === 0) {
cb();
}
}
});
this.getProjectId().then(
() => {
this.scheduleFlush();
if (--pendingOperations === 0) {
cb();
}
},
(err: Error) => {
this.logger.error(
'TraceWriter#initialize: Unable to acquire the project number',
'automatically from the GCP metadata service. Please provide a',
'valid project ID as environmental variable GCLOUD_PROJECT, or as',
`config.projectId passed to start. Original error: ${err}`);
cb(err);
});

this.getHostname((hostname) => {
this.getInstanceId((instanceId) => {
Expand Down Expand Up @@ -212,27 +213,14 @@ export class TraceWriter extends common.Service {
});
}

/**
* Returns the project ID if it has been cached and attempts to load
* it from the enviroment or network otherwise.
*/
getProjectId(cb: (err: Error|null, projectId?: string) => void) {
getProjectId() {
if (this.config.projectId) {
cb(null, this.config.projectId);
return;
return Promise.resolve(this.config.projectId);
}

gcpMetadata.project({property: 'project-id', headers})
.then((res) => {
cb(null, res.data); // project ID
})
.catch((err: AxiosError) => {
if (err.response && err.response.status === 503) {
err.message +=
' This may be due to a temporary server error; please try again later.';
}
cb(err);
});
return super.getProjectId().then((projectId) => {
this.config.projectId = projectId;
return projectId;
});
}

/**
Expand Down Expand Up @@ -264,14 +252,7 @@ export class TraceWriter extends common.Service {
* @param trace The trace to be queued.
*/
queueTrace(trace: Trace) {
this.getProjectId((err, projectId?) => {
if (err || !projectId) {
this.logger.info(
'TraceWriter#queueTrace: No project ID, dropping trace.');
return; // if we even reach this point, disabling traces is already
// imminent.
}

const afterProjectId = (projectId: string) => {
trace.projectId = projectId;
this.buffer.push(JSON.stringify(trace));
this.logger.info(
Expand All @@ -283,7 +264,23 @@ export class TraceWriter extends common.Service {
'TraceWriter#queueTrace: Trace buffer full, flushing.');
setImmediate(() => this.flushBuffer());
}
});
};
// TODO(kjin): We should always be following the 'else' path.
// Any test that doesn't mock the Trace Writer will assume that traces get
// buffered synchronously. We need to refactor those tests to remove that
// assumption before we can make this fix.
if (this.config.projectId) {
afterProjectId(this.config.projectId);
} else {
this.getProjectId().then(afterProjectId, (err: Error) => {
// Because failing to get a project ID means that the trace agent will
// get disabled, there is a very small window for this code path to be
// taken. For this reason we don't do anything more complex than just
// notifying that we are dropping the current trace.
this.logger.info(
'TraceWriter#queueTrace: No project ID, dropping trace.');
});
}
}

/**
Expand Down
17 changes: 9 additions & 8 deletions src/tracing.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
* limitations under the License.
*/

import * as common from '@google-cloud/common';
import {Logger, logger} from '@google-cloud/common';
import * as path from 'path';
import * as semver from 'semver';

Expand Down Expand Up @@ -42,7 +42,7 @@ export type NormalizedConfig =
*/
export class Tracing implements Component {
/** A logger. */
private readonly logger: common.Logger;
private readonly logger: Logger;
/** The configuration object for this instance. */
private readonly config: Forceable<NormalizedConfig>;

Expand All @@ -56,15 +56,16 @@ export class Tracing implements Component {
this.config = config;
let logLevel = config.enabled ? config.logLevel : 0;
// Clamp the logger level.
// TODO(kjin): When @google-cloud/common@0.19.2 is released, use
// Logger.LEVELS instead.
const defaultLevels = logger.LEVELS;
if (logLevel < 0) {
logLevel = 0;
} else if (logLevel >= common.logger.LEVELS.length) {
logLevel = common.logger.LEVELS.length - 1;
} else if (logLevel >= defaultLevels.length) {
logLevel = defaultLevels.length - 1;
}
this.logger = common.logger({
level: common.logger.LEVELS[logLevel],
tag: '@google-cloud/trace-agent'
});
this.logger = new Logger(
{level: defaultLevels[logLevel], tag: '@google-cloud/trace-agent'});
}


Expand Down
54 changes: 0 additions & 54 deletions src/types.d.ts
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,3 @@
// TODO(kjin): Unify these definitions with those of the Debugger Agent.

declare namespace NodeJS {
export interface Global {
_google_trace_agent: any;
Expand All @@ -18,58 +16,6 @@ declare namespace NodeJS {
}
}

declare module '@google-cloud/common' {
import * as request from 'request';

type LogFunction = (message: any, ...args: any[]) => void;

export interface Logger {
error: LogFunction;
warn: LogFunction;
info: LogFunction;
debug: LogFunction;
silly: LogFunction;
}

export interface LoggerOptions {
level?: string;
levels?: string[];
tag?: string;
}

export const logger: {
(options?: LoggerOptions | string): Logger;
LEVELS: string[];
};

export class Service {
constructor(config: ServiceConfig, options: ServiceAuthenticationConfig);
request(options: request.Options,
cb: (
err: Error | null,
body: any,
response: request.RequestResponse
) => void): void;
}

export interface ServiceConfig {
packageJson?: any;
projectIdRequired?: boolean;
baseUrl?: string;
scopes?: string[];
}

export interface ServiceAuthenticationConfig {
projectId?: string;
keyFilename?: string;
email?: string;
credentials?: {
client_email?: string;
private_key?: string;
};
}
}

declare module 'require-in-the-middle' {
namespace hook {
type Options = {
Expand Down
25 changes: 17 additions & 8 deletions test/logger.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,26 +14,35 @@
* limitations under the License.
*/

import {Logger, logger} from '@google-cloud/common';
import {Logger, logger, LoggerConfig} from '@google-cloud/common';

const PASS_THROUGH_LOG_LEVEL = Number(process.env.GCLOUD_TEST_LOG_LEVEL || 0);
// Capture the value of common.Logger so that we don't enter an infinite loop
// if common.Logger is wrapped elsewhere.
// tslint:disable-next-line:variable-name
const OriginalLogger = Logger;

// tslint:disable-next-line:no-any
type LoggerFunction = (message: any, ...args: any[]) => void;
type LoggerFunction<R> = (message: any, ...args: any[]) => R;

export class TestLogger implements Logger {
private logs:
{[k in keyof Logger]:
string[]} = {error: [], warn: [], info: [], debug: [], silly: []};
private innerLogger = logger({level: logger.LEVELS[PASS_THROUGH_LOG_LEVEL]});
export class TestLogger extends Logger {
private logs: {[k in keyof Logger]: string[]} =
{silent: [], error: [], warn: [], info: [], debug: [], silly: []};
private innerLogger =
new OriginalLogger({level: logger.LEVELS[PASS_THROUGH_LOG_LEVEL]});

private makeLoggerFn(logLevel: keyof Logger): LoggerFunction {
constructor(options?: Partial<LoggerConfig>) {
super(options);
}

private makeLoggerFn(logLevel: keyof Logger): LoggerFunction<this> {
// TODO(kjin): When we drop support for Node 4, use spread args.
const that = this;
return function(this: null) {
const args = Array.prototype.slice.call(arguments, 0);
that.logs[logLevel].push(args.join(' '));
that.innerLogger[logLevel].apply(this, args);
return that;
};
}

Expand Down
56 changes: 23 additions & 33 deletions test/test-agent-stopped.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,39 +14,34 @@
* limitations under the License.
*/

'use strict';
import * as assert from 'assert';
import * as http from 'http';
import * as traceTestModule from './trace';
import { pluginLoader, PluginLoaderState } from '../src/trace-plugin-loader';
import { TraceWriter } from '../src/trace-writer';
import { TraceAgent } from '../src/trace-api';

import './override-gcp-metadata';
describe('test-agent-stopped', () => {
class InitErrorTraceWriter extends TraceWriter {
getProjectId() {
return Promise.reject(new Error('foo'));
}
}

var assert = require('assert');
var http = require('http');
var nock = require('nock');
var trace = require('../..');
var pluginLoader = require('../src/trace-plugin-loader').pluginLoader;
var PluginLoaderState = require('../src/trace-plugin-loader').PluginLoaderState;

describe('test-agent-stopped', function() {
var agent;
var savedProject;

before(function(done) {
savedProject = process.env.GCLOUD_PROJECT;

var scope = nock('http://metadata.google.internal')
.get('/computeMetadata/v1/project/project-id')
.reply(404);
delete process.env.GCLOUD_PROJECT;
agent = trace.start();
// Wait 200ms for agent to fail getting remote project id.
setTimeout(function() {
assert.ok(!agent.isActive());
scope.done();
before((done) => {
traceTestModule.setPluginLoaderForTest();
traceTestModule.setTraceWriterForTest(InitErrorTraceWriter);
traceTestModule.start();
// Wait for agent to fail getting remote project id.
setImmediate(() => {
assert.ok(!(traceTestModule.get() as TraceAgent).isActive());
done();
}, 200);
});
});

after(function() {
process.env.GCLOUD_PROJECT = savedProject;
after(() => {
traceTestModule.setPluginLoaderForTest(traceTestModule.TestPluginLoader);
traceTestModule.setTraceWriterForTest(traceTestModule.TestTraceWriter);
});

it('deactivates the plugin loader', () => {
Expand All @@ -55,7 +50,6 @@ describe('test-agent-stopped', function() {

describe('express', function() {
it('should not break if no project number is found', function(done) {
assert.ok(!agent.isActive());
var app = require('./plugins/fixtures/express4')();
app.get('/', function (req, res) {
res.send('hi');
Expand All @@ -76,7 +70,6 @@ describe('test-agent-stopped', function() {

describe('hapi', function() {
it('should not break if no project number is found', function(done) {
assert.ok(!agent.isActive());
var hapi = require('./plugins/fixtures/hapi8');
var server = new hapi.Server();
server.connection({ port: 8081 });
Expand All @@ -103,7 +96,6 @@ describe('test-agent-stopped', function() {

describe('restify', function() {
it('should not break if no project number is found', function(done) {
assert.ok(!agent.isActive());
var restify = require('./plugins/fixtures/restify4');
var server = restify.createServer();
server.get('/', function (req, res, next) {
Expand All @@ -128,5 +120,3 @@ describe('test-agent-stopped', function() {
});
});
});

export default {};
Loading

0 comments on commit 3f3f667

Please sign in to comment.