Skip to content
This repository was archived by the owner on Jan 21, 2026. It is now read-only.

Commit 3f3f667

Browse files
kjinkjin
authored
fix: update to @google-cloud/[email protected] (#772)
* fix: update to @google-cloud/[email protected] * chore: update package-lock.json * test: update most of the tests that fail * test: rewrite test-trace-writer * refactor: address comments
1 parent 7b9e48f commit 3f3f667

18 files changed

Lines changed: 844 additions & 1490 deletions

‎package-lock.json‎

Lines changed: 337 additions & 731 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎package.json‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -92,7 +92,7 @@
9292
"typescript": "~2.7.2"
9393
},
9494
"dependencies": {
95-
"@google-cloud/common": "^0.17.0",
95+
"@google-cloud/common": "^0.19.1",
9696
"builtin-modules": "^2.0.0",
9797
"continuation-local-storage": "^3.2.1",
9898
"extend": "^3.0.0",

‎src/trace-writer.ts‎

Lines changed: 43 additions & 46 deletions
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,7 @@ headers[Constants.TRACE_AGENT_REQUEST_HEADER] = 1;
3636
/* A list of scopes needed to operate with the trace API */
3737
const SCOPES: string[] = ['https://www.googleapis.com/auth/trace.append'];
3838

39-
export interface TraceWriterConfig extends common.ServiceAuthenticationConfig {
39+
export interface TraceWriterConfig extends common.GoogleAuthOptions {
4040
projectId?: string;
4141
onUncaughtException: string;
4242
bufferSize: number;
@@ -82,7 +82,9 @@ export class TraceWriter extends common.Service {
8282
config);
8383

8484
this.logger = logger;
85-
this.config = config;
85+
// Clone the config object
86+
this.config = {...config};
87+
this.config.serviceContext = {...this.config.serviceContext};
8688
this.buffer = [];
8789
this.defaultLabels = {};
8890

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

122124
// Schedule periodic flushing of the buffer, but only if we are able to get
123125
// the project number (potentially from the network.)
124-
this.getProjectId((err: Error|null, project?: string) => {
125-
if (err) {
126-
this.logger.error(
127-
'TraceWriter#initialize: Unable to acquire the project number',
128-
'automatically from the GCP metadata service. Please provide a',
129-
'valid project ID as environmental variable GCLOUD_PROJECT, or as',
130-
`config.projectId passed to start. Original error: ${err}`);
131-
cb(err);
132-
} else {
133-
this.config.projectId = project;
134-
this.scheduleFlush();
135-
if (--pendingOperations === 0) {
136-
cb();
137-
}
138-
}
139-
});
126+
this.getProjectId().then(
127+
() => {
128+
this.scheduleFlush();
129+
if (--pendingOperations === 0) {
130+
cb();
131+
}
132+
},
133+
(err: Error) => {
134+
this.logger.error(
135+
'TraceWriter#initialize: Unable to acquire the project number',
136+
'automatically from the GCP metadata service. Please provide a',
137+
'valid project ID as environmental variable GCLOUD_PROJECT, or as',
138+
`config.projectId passed to start. Original error: ${err}`);
139+
cb(err);
140+
});
140141

141142
this.getHostname((hostname) => {
142143
this.getInstanceId((instanceId) => {
@@ -212,27 +213,14 @@ export class TraceWriter extends common.Service {
212213
});
213214
}
214215

215-
/**
216-
* Returns the project ID if it has been cached and attempts to load
217-
* it from the enviroment or network otherwise.
218-
*/
219-
getProjectId(cb: (err: Error|null, projectId?: string) => void) {
216+
getProjectId() {
220217
if (this.config.projectId) {
221-
cb(null, this.config.projectId);
222-
return;
218+
return Promise.resolve(this.config.projectId);
223219
}
224-
225-
gcpMetadata.project({property: 'project-id', headers})
226-
.then((res) => {
227-
cb(null, res.data); // project ID
228-
})
229-
.catch((err: AxiosError) => {
230-
if (err.response && err.response.status === 503) {
231-
err.message +=
232-
' This may be due to a temporary server error; please try again later.';
233-
}
234-
cb(err);
235-
});
220+
return super.getProjectId().then((projectId) => {
221+
this.config.projectId = projectId;
222+
return projectId;
223+
});
236224
}
237225

238226
/**
@@ -264,14 +252,7 @@ export class TraceWriter extends common.Service {
264252
* @param trace The trace to be queued.
265253
*/
266254
queueTrace(trace: Trace) {
267-
this.getProjectId((err, projectId?) => {
268-
if (err || !projectId) {
269-
this.logger.info(
270-
'TraceWriter#queueTrace: No project ID, dropping trace.');
271-
return; // if we even reach this point, disabling traces is already
272-
// imminent.
273-
}
274-
255+
const afterProjectId = (projectId: string) => {
275256
trace.projectId = projectId;
276257
this.buffer.push(JSON.stringify(trace));
277258
this.logger.info(
@@ -283,7 +264,23 @@ export class TraceWriter extends common.Service {
283264
'TraceWriter#queueTrace: Trace buffer full, flushing.');
284265
setImmediate(() => this.flushBuffer());
285266
}
286-
});
267+
};
268+
// TODO(kjin): We should always be following the 'else' path.
269+
// Any test that doesn't mock the Trace Writer will assume that traces get
270+
// buffered synchronously. We need to refactor those tests to remove that
271+
// assumption before we can make this fix.
272+
if (this.config.projectId) {
273+
afterProjectId(this.config.projectId);
274+
} else {
275+
this.getProjectId().then(afterProjectId, (err: Error) => {
276+
// Because failing to get a project ID means that the trace agent will
277+
// get disabled, there is a very small window for this code path to be
278+
// taken. For this reason we don't do anything more complex than just
279+
// notifying that we are dropping the current trace.
280+
this.logger.info(
281+
'TraceWriter#queueTrace: No project ID, dropping trace.');
282+
});
283+
}
287284
}
288285

289286
/**

‎src/tracing.ts‎

Lines changed: 9 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@
1414
* limitations under the License.
1515
*/
1616

17-
import * as common from '@google-cloud/common';
17+
import {Logger, logger} from '@google-cloud/common';
1818
import * as path from 'path';
1919
import * as semver from 'semver';
2020

@@ -42,7 +42,7 @@ export type NormalizedConfig =
4242
*/
4343
export class Tracing implements Component {
4444
/** A logger. */
45-
private readonly logger: common.Logger;
45+
private readonly logger: Logger;
4646
/** The configuration object for this instance. */
4747
private readonly config: Forceable<NormalizedConfig>;
4848

@@ -56,15 +56,16 @@ export class Tracing implements Component {
5656
this.config = config;
5757
let logLevel = config.enabled ? config.logLevel : 0;
5858
// Clamp the logger level.
59+
// TODO(kjin): When @google-cloud/[email protected] is released, use
60+
// Logger.LEVELS instead.
61+
const defaultLevels = logger.LEVELS;
5962
if (logLevel < 0) {
6063
logLevel = 0;
61-
} else if (logLevel >= common.logger.LEVELS.length) {
62-
logLevel = common.logger.LEVELS.length - 1;
64+
} else if (logLevel >= defaultLevels.length) {
65+
logLevel = defaultLevels.length - 1;
6366
}
64-
this.logger = common.logger({
65-
level: common.logger.LEVELS[logLevel],
66-
tag: '@google-cloud/trace-agent'
67-
});
67+
this.logger = new Logger(
68+
{level: defaultLevels[logLevel], tag: '@google-cloud/trace-agent'});
6869
}
6970

7071

‎src/types.d.ts‎

Lines changed: 0 additions & 54 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,3 @@
1-
// TODO(kjin): Unify these definitions with those of the Debugger Agent.
2-
31
declare namespace NodeJS {
42
export interface Global {
53
_google_trace_agent: any;
@@ -18,58 +16,6 @@ declare namespace NodeJS {
1816
}
1917
}
2018

21-
declare module '@google-cloud/common' {
22-
import * as request from 'request';
23-
24-
type LogFunction = (message: any, ...args: any[]) => void;
25-
26-
export interface Logger {
27-
error: LogFunction;
28-
warn: LogFunction;
29-
info: LogFunction;
30-
debug: LogFunction;
31-
silly: LogFunction;
32-
}
33-
34-
export interface LoggerOptions {
35-
level?: string;
36-
levels?: string[];
37-
tag?: string;
38-
}
39-
40-
export const logger: {
41-
(options?: LoggerOptions | string): Logger;
42-
LEVELS: string[];
43-
};
44-
45-
export class Service {
46-
constructor(config: ServiceConfig, options: ServiceAuthenticationConfig);
47-
request(options: request.Options,
48-
cb: (
49-
err: Error | null,
50-
body: any,
51-
response: request.RequestResponse
52-
) => void): void;
53-
}
54-
55-
export interface ServiceConfig {
56-
packageJson?: any;
57-
projectIdRequired?: boolean;
58-
baseUrl?: string;
59-
scopes?: string[];
60-
}
61-
62-
export interface ServiceAuthenticationConfig {
63-
projectId?: string;
64-
keyFilename?: string;
65-
email?: string;
66-
credentials?: {
67-
client_email?: string;
68-
private_key?: string;
69-
};
70-
}
71-
}
72-
7319
declare module 'require-in-the-middle' {
7420
namespace hook {
7521
type Options = {

‎test/logger.ts‎

Lines changed: 17 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -14,26 +14,35 @@
1414
* limitations under the License.
1515
*/
1616

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

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

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

24-
export class TestLogger implements Logger {
25-
private logs:
26-
{[k in keyof Logger]:
27-
string[]} = {error: [], warn: [], info: [], debug: [], silly: []};
28-
private innerLogger = logger({level: logger.LEVELS[PASS_THROUGH_LOG_LEVEL]});
28+
export class TestLogger extends Logger {
29+
private logs: {[k in keyof Logger]: string[]} =
30+
{silent: [], error: [], warn: [], info: [], debug: [], silly: []};
31+
private innerLogger =
32+
new OriginalLogger({level: logger.LEVELS[PASS_THROUGH_LOG_LEVEL]});
2933

30-
private makeLoggerFn(logLevel: keyof Logger): LoggerFunction {
34+
constructor(options?: Partial<LoggerConfig>) {
35+
super(options);
36+
}
37+
38+
private makeLoggerFn(logLevel: keyof Logger): LoggerFunction<this> {
3139
// TODO(kjin): When we drop support for Node 4, use spread args.
3240
const that = this;
3341
return function(this: null) {
3442
const args = Array.prototype.slice.call(arguments, 0);
3543
that.logs[logLevel].push(args.join(' '));
3644
that.innerLogger[logLevel].apply(this, args);
45+
return that;
3746
};
3847
}
3948

‎test/test-agent-stopped.ts‎

Lines changed: 23 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -14,39 +14,34 @@
1414
* limitations under the License.
1515
*/
1616

17-
'use strict';
17+
import * as assert from 'assert';
18+
import * as http from 'http';
19+
import * as traceTestModule from './trace';
20+
import { pluginLoader, PluginLoaderState } from '../src/trace-plugin-loader';
21+
import { TraceWriter } from '../src/trace-writer';
22+
import { TraceAgent } from '../src/trace-api';
1823

19-
import './override-gcp-metadata';
24+
describe('test-agent-stopped', () => {
25+
class InitErrorTraceWriter extends TraceWriter {
26+
getProjectId() {
27+
return Promise.reject(new Error('foo'));
28+
}
29+
}
2030

21-
var assert = require('assert');
22-
var http = require('http');
23-
var nock = require('nock');
24-
var trace = require('../..');
25-
var pluginLoader = require('../src/trace-plugin-loader').pluginLoader;
26-
var PluginLoaderState = require('../src/trace-plugin-loader').PluginLoaderState;
27-
28-
describe('test-agent-stopped', function() {
29-
var agent;
30-
var savedProject;
31-
32-
before(function(done) {
33-
savedProject = process.env.GCLOUD_PROJECT;
34-
35-
var scope = nock('http://metadata.google.internal')
36-
.get('/computeMetadata/v1/project/project-id')
37-
.reply(404);
38-
delete process.env.GCLOUD_PROJECT;
39-
agent = trace.start();
40-
// Wait 200ms for agent to fail getting remote project id.
41-
setTimeout(function() {
42-
assert.ok(!agent.isActive());
43-
scope.done();
31+
before((done) => {
32+
traceTestModule.setPluginLoaderForTest();
33+
traceTestModule.setTraceWriterForTest(InitErrorTraceWriter);
34+
traceTestModule.start();
35+
// Wait for agent to fail getting remote project id.
36+
setImmediate(() => {
37+
assert.ok(!(traceTestModule.get() as TraceAgent).isActive());
4438
done();
45-
}, 200);
39+
});
4640
});
4741

48-
after(function() {
49-
process.env.GCLOUD_PROJECT = savedProject;
42+
after(() => {
43+
traceTestModule.setPluginLoaderForTest(traceTestModule.TestPluginLoader);
44+
traceTestModule.setTraceWriterForTest(traceTestModule.TestTraceWriter);
5045
});
5146

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

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

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

10497
describe('restify', function() {
10598
it('should not break if no project number is found', function(done) {
106-
assert.ok(!agent.isActive());
10799
var restify = require('./plugins/fixtures/restify4');
108100
var server = restify.createServer();
109101
server.get('/', function (req, res, next) {
@@ -128,5 +120,3 @@ describe('test-agent-stopped', function() {
128120
});
129121
});
130122
});
131-
132-
export default {};

0 commit comments

Comments
 (0)