Skip to content

Commit 0d63312

Browse files
surbhigarg92surbhigarg92
authored
fix: Correctly determine project ID for metrics export (#2427)
* fix: Metrics Export with projectId from Credentials * test cases * fixed the metricstracerfactory instance, it is per projectid now * review comments
1 parent 89865ae commit 0d63312

13 files changed

+124
-75
lines changed

‎src/index.ts‎

Lines changed: 39 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -314,6 +314,7 @@ class Spanner extends GrpcService {
314314
instances_: Map<string, Instance>;
315315
instanceConfigs_: Map<string, InstanceConfig>;
316316
projectIdReplaced_: boolean;
317+
projectId_?: string;
317318
projectFormattedName_: string;
318319
commonHeaders_: {[k: string]: string};
319320
routeToLeaderEnabled = true;
@@ -500,6 +501,7 @@ class Spanner extends GrpcService {
500501
ensureInitialContextManagerSet();
501502
this._nthClientId = nextSpannerClientId();
502503
this._universeDomain = universeEndpoint;
504+
this.projectId_ = options.projectId;
503505
this.configureMetrics_(options.disableBuiltInMetrics);
504506
}
505507

@@ -1620,19 +1622,44 @@ class Spanner extends GrpcService {
16201622
* Setup the OpenTelemetry metrics capturing for service metrics to Google Cloud Monitoring.
16211623
*/
16221624
configureMetrics_(disableBuiltInMetrics?: boolean) {
1625+
// Only enable metrics if not explicitly disabled and we are not using
1626+
// insecure credentials.
16231627
const metricsEnabled =
16241628
process.env.SPANNER_DISABLE_BUILTIN_METRICS !== 'true' &&
16251629
!disableBuiltInMetrics &&
16261630
!this._isInSecureCredentials;
1627-
MetricsTracerFactory.enabled = metricsEnabled;
16281631
if (metricsEnabled) {
1629-
const factory = MetricsTracerFactory.getInstance(this.projectId);
1630-
const periodicReader = new PeriodicExportingMetricReader({
1631-
exporter: new CloudMonitoringMetricsExporter({auth: this.auth}),
1632-
exportIntervalMillis: 60000,
1633-
});
1634-
// Retrieve the MeterProvider to trigger construction
1635-
factory!.getMeterProvider([periodicReader]);
1632+
try {
1633+
this.auth.getProjectId((err, projectId) => {
1634+
if (err || !projectId) {
1635+
console.error(
1636+
'Unable to get Project Id for client side metrics, will skip exporting client' +
1637+
' side metrics' +
1638+
err,
1639+
);
1640+
return;
1641+
}
1642+
1643+
MetricsTracerFactory.enabled = metricsEnabled;
1644+
this.projectId_ = projectId;
1645+
const factory = MetricsTracerFactory.getInstance(projectId);
1646+
const periodicReader = new PeriodicExportingMetricReader({
1647+
exporter: new CloudMonitoringMetricsExporter(
1648+
{auth: this.auth},
1649+
projectId,
1650+
),
1651+
exportIntervalMillis: 60000,
1652+
});
1653+
// Retrieve the MeterProvider to trigger construction
1654+
factory!.getMeterProvider([periodicReader]);
1655+
});
1656+
} catch (err) {
1657+
console.error(
1658+
'Unable to configure client side metrics, will skip exporting client' +
1659+
' side metrics' +
1660+
err,
1661+
);
1662+
}
16361663
}
16371664
}
16381665

@@ -1782,9 +1809,9 @@ class Spanner extends GrpcService {
17821809
// eslint-disable-next-line @typescript-eslint/no-explicit-any
17831810
request(config: any, callback?: any): any {
17841811
let metricsTracer: MetricsTracer | null = null;
1785-
if (config.client === 'SpannerClient') {
1812+
if (config.client === 'SpannerClient' && this.projectId_) {
17861813
metricsTracer =
1787-
MetricsTracerFactory?.getInstance()?.createMetricsTracer(
1814+
MetricsTracerFactory?.getInstance(this.projectId_)?.createMetricsTracer(
17881815
config.method,
17891816
config.reqOpts.session ?? config.reqOpts.database,
17901817
config.headers['x-goog-spanner-request-id'],
@@ -1846,9 +1873,9 @@ class Spanner extends GrpcService {
18461873
// eslint-disable-next-line @typescript-eslint/no-explicit-any
18471874
requestStream(config): any {
18481875
let metricsTracer: MetricsTracer | null = null;
1849-
if (config.client === 'SpannerClient') {
1876+
if (config.client === 'SpannerClient' && this.projectId_) {
18501877
metricsTracer =
1851-
MetricsTracerFactory?.getInstance()?.createMetricsTracer(
1878+
MetricsTracerFactory?.getInstance(this.projectId_)?.createMetricsTracer(
18521879
config.method,
18531880
config.reqOpts.session ?? config.reqOpts.database,
18541881
config.headers['x-goog-spanner-request-id'],

‎src/metrics/interceptor.ts‎

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,15 @@ export const MetricInterceptor = (options, nextCall) => {
3030
return new grpc.InterceptingCall(nextCall(options), {
3131
start: function (metadata, listener, next) {
3232
// Record attempt metric on request start
33-
const factory = MetricsTracerFactory.getInstance();
33+
const resourcePrefix = metadata.get(
34+
'google-cloud-resource-prefix',
35+
)[0] as string;
36+
const match = resourcePrefix?.match(/^projects\/([^/]+)\//);
37+
const projectId = match ? match[1] : undefined;
38+
let factory;
39+
if (projectId) {
40+
factory = MetricsTracerFactory.getInstance(projectId);
41+
}
3442
const requestId = metadata.get('x-goog-spanner-request-id')[0] as string;
3543
const metricsTracer = factory?.getCurrentTracer(requestId);
3644
metricsTracer?.recordAttemptStart();

‎src/metrics/metrics-tracer-factory.ts‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -56,7 +56,6 @@ export class MetricsTracerFactory {
5656
private _currentOperationTracers = new Map();
5757
private _currentOperationLastUpdatedMs = new Map();
5858
private _intervalTracerCleanup: NodeJS.Timeout;
59-
public static _readers: MetricReader[] = [];
6059
public static enabled = true;
6160

6261
/**
@@ -101,7 +100,7 @@ export class MetricsTracerFactory {
101100
* @param projectId Optional GCP project ID for the factory instantiation. Does nothing for subsequent calls.
102101
* @returns The singleton MetricsTracerFactory instance or null if disabled.
103102
*/
104-
public static getInstance(projectId = ''): MetricsTracerFactory | null {
103+
public static getInstance(projectId: string): MetricsTracerFactory | null {
105104
if (!MetricsTracerFactory.enabled) {
106105
return null;
107106
}
@@ -110,6 +109,7 @@ export class MetricsTracerFactory {
110109
if (MetricsTracerFactory._instance === null) {
111110
MetricsTracerFactory._instance = new MetricsTracerFactory(projectId);
112111
}
112+
113113
return MetricsTracerFactory!._instance;
114114
}
115115

@@ -128,7 +128,6 @@ export class MetricsTracerFactory {
128128
[Constants.MONITORED_RES_LABEL_KEY_INSTANCE]: 'unknown',
129129
[Constants.MONITORED_RES_LABEL_KEY_INSTANCE_CONFIG]: 'unknown',
130130
});
131-
MetricsTracerFactory._readers = readers;
132131
this._meterProvider = new MeterProvider({
133132
resource: resource,
134133
readers: readers,
@@ -142,7 +141,7 @@ export class MetricsTracerFactory {
142141
/**
143142
* Resets the singleton instance of the MetricsTracerFactory.
144143
*/
145-
public static async resetInstance() {
144+
public static async resetInstance(projectId?: string) {
146145
clearInterval(MetricsTracerFactory._instance?._intervalTracerCleanup);
147146
await MetricsTracerFactory._instance?.resetMeterProvider();
148147
MetricsTracerFactory._instance = null;
@@ -250,6 +249,7 @@ export class MetricsTracerFactory {
250249
MetricsTracerFactory.enabled,
251250
database,
252251
instance,
252+
this._projectId,
253253
method,
254254
operationRequest,
255255
);

‎src/metrics/metrics-tracer.ts‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -163,6 +163,7 @@ export class MetricsTracer {
163163
public enabled: boolean,
164164
private _database: string,
165165
private _instance: string,
166+
private _projectId: string,
166167
private _methodName: string,
167168
private _request: string,
168169
) {
@@ -276,7 +277,9 @@ export class MetricsTracer {
276277
operationLatencyMilliseconds,
277278
operationAttributes,
278279
);
279-
MetricsTracerFactory.getInstance()!.clearCurrentTracer(this._request);
280+
MetricsTracerFactory.getInstance(this._projectId)!.clearCurrentTracer(
281+
this._request,
282+
);
280283
}
281284

282285
/**

‎src/metrics/spanner-metrics-exporter.ts‎

Lines changed: 7 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -28,19 +28,15 @@ export const MAX_BATCH_EXPORT_SIZE = 200;
2828
* Format and sends metrics information to Google Cloud Monitoring.
2929
*/
3030
export class CloudMonitoringMetricsExporter implements PushMetricExporter {
31-
private _projectId: string | void | Promise<string | void>;
31+
private _projectId: string;
3232
private _lastExported: Date = new Date(0);
3333
private readonly _client: MetricServiceClient;
3434
private _metricsExportFailureLogged = false;
3535

36-
constructor({auth}: ExporterOptions) {
36+
constructor({auth}: ExporterOptions, projectId: string) {
3737
this._client = new MetricServiceClient({auth: auth});
3838

39-
// Start this async process as early as possible. It will be
40-
// awaited on the first export because constructors are synchronous
41-
this._projectId = auth.getProjectId().catch(err => {
42-
console.error(err);
43-
});
39+
this._projectId = projectId;
4440
}
4541

4642
/**
@@ -83,18 +79,10 @@ export class CloudMonitoringMetricsExporter implements PushMetricExporter {
8379
private async _exportAsync(
8480
resourceMetrics: ResourceMetrics,
8581
): Promise<ExportResult> {
86-
if (this._projectId instanceof Promise) {
87-
this._projectId = await this._projectId;
88-
}
89-
90-
if (!this._projectId) {
91-
const error = new Error('expecting a non-blank ProjectID');
92-
console.error(error.message);
93-
return {code: ExportResultCode.FAILED, error};
94-
}
95-
96-
const timeSeriesList =
97-
transformResourceMetricToTimeSeriesArray(resourceMetrics);
82+
const timeSeriesList = transformResourceMetricToTimeSeriesArray(
83+
resourceMetrics,
84+
this._projectId,
85+
);
9886

9987
let failure: {sendFailed: false} | {sendFailed: true; error: Error} = {
10088
sendFailed: false,

‎src/metrics/transform.ts‎

Lines changed: 11 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -90,6 +90,7 @@ function _transformValueType(metric: MetricData): ValueType {
9090
*/
9191
export function transformResourceMetricToTimeSeriesArray(
9292
resourceMetrics: ResourceMetrics,
93+
projectId: string,
9394
) {
9495
const resource = resourceMetrics?.resource;
9596
const scopeMetrics = resourceMetrics?.scopeMetrics;
@@ -107,7 +108,7 @@ export function transformResourceMetricToTimeSeriesArray(
107108
// Flatmap the data points in each metric to create a TimeSeries for each point
108109
.flatMap(metric =>
109110
metric.dataPoints.flatMap(dataPoint =>
110-
_createTimeSeries(metric, dataPoint, resource),
111+
_createTimeSeries(metric, dataPoint, resource, projectId),
111112
),
112113
)
113114
);
@@ -119,14 +120,15 @@ export function transformResourceMetricToTimeSeriesArray(
119120
function _createTimeSeries<T>(
120121
metric: MetricData,
121122
dataPoint: DataPoint<T>,
122-
resource?: Resource,
123+
resource: Resource,
124+
projectId: string,
123125
) {
124126
const type = path.posix.join(CLIENT_METRICS_PREFIX, metric.descriptor.name);
125127
const resourceLabels = resource
126-
? _extractLabels(resource)
128+
? _extractLabels(resource, projectId)
127129
: {metricLabels: {}, monitoredResourceLabels: {}};
128130

129-
const dataLabels = _extractLabels(dataPoint);
131+
const dataLabels = _extractLabels(dataPoint, projectId);
130132

131133
const labels = {
132134
...resourceLabels.metricLabels,
@@ -205,8 +207,11 @@ function _transformPoint<T>(metric: MetricData, dataPoint: DataPoint<T>) {
205207
}
206208

207209
/** Extracts metric and monitored resource labels from data point */
208-
function _extractLabels<T>({attributes = {}}: DataPoint<T> | Resource) {
209-
const factory = MetricsTracerFactory.getInstance();
210+
function _extractLabels<T>(
211+
{attributes = {}}: DataPoint<T> | Resource,
212+
projectId: string,
213+
) {
214+
const factory = MetricsTracerFactory.getInstance(projectId);
210215
// Add Client name and Client UID metric labels
211216
attributes[METRIC_LABEL_KEY_CLIENT_UID] =
212217
factory?.clientUid ?? UNKNOWN_ATTRIBUTE;

‎test/index.ts‎

Lines changed: 21 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -51,20 +51,16 @@ assert.strictEqual(CLOUD_RESOURCE_HEADER, 'google-cloud-resource-prefix');
5151
const apiConfig = require('../src/spanner_grpc_config.json');
5252

5353
async function disableMetrics(sandbox: sinon.SinonSandbox) {
54-
if (
55-
Object.prototype.hasOwnProperty.call(
56-
process.env,
57-
'SPANNER_DISABLE_BUILTIN_METRICS',
58-
)
59-
) {
60-
sandbox.replace(process.env, 'SPANNER_DISABLE_BUILTIN_METRICS', 'true');
61-
} else {
62-
sandbox.define(process.env, 'SPANNER_DISABLE_BUILTIN_METRICS', 'true');
63-
}
54+
sandbox.stub(process.env, 'SPANNER_DISABLE_BUILTIN_METRICS').value('true');
6455
await MetricsTracerFactory.resetInstance();
6556
MetricsTracerFactory.enabled = false;
6657
}
6758

59+
async function enableMetrics(sandbox: sinon.SinonSandbox) {
60+
sandbox.stub(process.env, 'SPANNER_DISABLE_BUILTIN_METRICS').value('false');
61+
await MetricsTracerFactory.resetInstance();
62+
}
63+
6864
function getFake(obj: {}) {
6965
return obj as {
7066
calledWith_: IArguments;
@@ -133,7 +129,14 @@ const fakeV1: any = {
133129
function fakeGoogleAuth() {
134130
return {
135131
calledWith_: arguments,
136-
getProjectId: () => Promise.resolve('project-id'),
132+
getProjectId: function (callback) {
133+
if (callback) {
134+
callback(null, 'project-id');
135+
return;
136+
} else {
137+
return Promise.resolve('project-id');
138+
}
139+
},
137140
};
138141
}
139142

@@ -328,6 +331,13 @@ describe('Spanner', () => {
328331
assert.strictEqual(spanner.routeToLeaderEnabled, false);
329332
});
330333

334+
it('should configure metrics if project Id is not passed', async () => {
335+
await enableMetrics(sandbox);
336+
const spanner = new Spanner();
337+
assert.strictEqual(MetricsTracerFactory.enabled, true);
338+
await disableMetrics(sandbox);
339+
});
340+
331341
it('should optionally accept disableBuiltInMetrics', () => {
332342
const spanner = new Spanner({disableBuiltInMetrics: true});
333343
assert.strictEqual(MetricsTracerFactory.enabled, false);

‎test/metrics/interceptor.ts‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -117,6 +117,10 @@ describe('MetricInterceptor', () => {
117117
},
118118
};
119119
testMetadata = new grpc.Metadata();
120+
testMetadata.set(
121+
'google-cloud-resource-prefix',
122+
'projects/test-project/instances/instance/databases/database-1',
123+
);
120124
});
121125

122126
afterEach(() => {

‎test/metrics/metrics-tracer-factory.ts‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -93,7 +93,7 @@ describe('MetricsTracerFactory', () => {
9393
});
9494

9595
it('should use the set meter provider', async () => {
96-
const factory = MetricsTracerFactory.getInstance();
96+
const factory = MetricsTracerFactory.getInstance('project-id');
9797
const tracer = factory!.createMetricsTracer(
9898
'some-method',
9999
'projects/project/instances/instance/databases/database',
@@ -126,7 +126,7 @@ describe('MetricsTracerFactory', () => {
126126
});
127127

128128
it('should initialize metric instruments when enabled', () => {
129-
const factory = MetricsTracerFactory.getInstance();
129+
const factory = MetricsTracerFactory.getInstance('project-id');
130130

131131
assert.deepStrictEqual(factory!.instrumentAttemptLatency, {
132132
record: recordAttemptLatencyStub,
@@ -149,7 +149,7 @@ describe('MetricsTracerFactory', () => {
149149
});
150150

151151
it('should create a MetricsTracer instance', () => {
152-
const factory = MetricsTracerFactory.getInstance();
152+
const factory = MetricsTracerFactory.getInstance('project-id');
153153
const tracer = factory!.createMetricsTracer(
154154
'some-method',
155155
'method-name',
@@ -159,7 +159,7 @@ describe('MetricsTracerFactory', () => {
159159
});
160160

161161
it('should correctly set default attributes', () => {
162-
const factory = MetricsTracerFactory.getInstance();
162+
const factory = MetricsTracerFactory.getInstance('project-id');
163163
const tracer = factory!.createMetricsTracer(
164164
'test-method',
165165
'projects/project/instances/instance/databases/database',

0 commit comments

Comments
 (0)