Address review comments

Author: ravjotbrar

src/metrics/README.md

@@ -4,14 +4,14 @@ The custom metric exporter, as defined in [spanner-metrics-exporter.ts](./spanne
 ## Filtering Criteria
 The exporter filters metrics based on the following conditions, utilizing values defined in [constants.ts](./constants.ts):
 
-* Metrics with a scope set to `gax-nodejs`.
+* Metrics with a scope set to `spanner-nodejs`.
 * Metrics with one of the following predefined names:
   * `attempt_latencies`
   * `attempt_count`
   * `operation_latencies`
   * `operation_count`
   * `gfe_latency`
-  * `gfe_missing_header_count`
+  * `gfe_connectivity_error_count`
 
 ## Service Endpoint
 The exporter sends metrics to the Google Cloud Monitoring [service endpoint](https://cloud.google.com/python/docs/reference/monitoring/latest/google.cloud.monitoring_v3.services.metric_service.MetricServiceClient#google_cloud_monitoring_v3_services_metric_service_MetricServiceClient_create_service_time_series), distinct from the regular client endpoint. This service endpoint operates under a different quota limit than the user endpoint and features an additional server-side filter that only permits a predefined set of metrics to pass through.

src/metrics/external-types.ts

@@ -12,20 +12,13 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-import {GoogleAuthOptions} from 'google-auth-library';
+import {GoogleAuth} from 'google-auth-library';
 
 export interface ExporterOptions {
   /**
    * Optional authentication options for Google services.
    */
-  authOptions?: GoogleAuthOptions;
-  /**
-   * Prefix prepended to OpenTelemetry metric names when writing to Cloud Monitoring. See
-   * https://cloud.google.com/monitoring/custom-metrics#identifier for more details.
-   *
-   * Optional, default is `workload.googleapis.com`.
-   */
-  prefix?: string;
+  auth: GoogleAuth;
   /**
    * Add a custom user agent and version strings to all monitoring exports
    */

src/metrics/spanner-metrics-exporter.ts

@@ -23,6 +23,7 @@ import {GoogleAuth, JWT} from 'google-auth-library';
 import {monitoring_v3} from 'googleapis';
 import {transformResourceMetricToTimeSeriesArray} from './transform';
 import {version} from '../../package.json';
+import {status} from '@grpc/grpc-js';
 
 // Stackdriver Monitoring v3 only accepts up to 200 TimeSeries per
 // CreateTimeSeries call.
@@ -54,14 +55,8 @@ export class CloudMonitoringMetricsExporter implements PushMetricExporter {
 
   private _monitoring: monitoring_v3.Monitoring;
 
-  constructor({authOptions, userAgent, apiEndpoint}: ExporterOptions = {}) {
-    this._auth = new GoogleAuth({
-      credentials: authOptions?.credentials,
-      keyFile: authOptions?.keyFile,
-      keyFilename: authOptions?.keyFilename,
-      projectId: authOptions?.projectId,
-      scopes: ['https://www.googleapis.com/auth/cloud-platform'],
-    });
+  constructor({auth, userAgent, apiEndpoint}: ExporterOptions) {
+    this._auth = auth;
 
     this._monitoring = new monitoring_v3.Monitoring({
       rootUrl: `https://${apiEndpoint || DEFAULT_API_ENDPOINT}`,
@@ -125,18 +120,21 @@ export class CloudMonitoringMetricsExporter implements PushMetricExporter {
     };
     await Promise.all(
       this._partitionList(timeSeriesList, MAX_BATCH_EXPORT_SIZE).map(
-        async batchedTimeSeries => {
-          try {
-            await this._sendTimeSeries(batchedTimeSeries);
-          } catch (e) {
-            const err = asError(e);
-            err.message = `Send TimeSeries failed: ${err.message}`;
-            failure = {sendFailed: true, error: err};
-            console.error(`ERROR: ${err.message}`);
-          }
-        }
+        async batchedTimeSeries => this._sendTimeSeries(batchedTimeSeries)
       )
-    );
+    ).catch(e => {
+      const error = e as {code: number};
+      if (error.code === status.PERMISSION_DENIED) {
+        console.warn(
+          `Need monitoring metric writer permission on project ${this._projectId}. Follow https://cloud.google.com/spanner/docs/view-manage-client-side-metrics#access-client-side-metrics to set up permissions`
+        );
+      }
+      const err = asError(e);
+      err.message = `Send TimeSeries failed: ${err.message}`;
+      failure = {sendFailed: true, error: err};
+      console.error(`ERROR: ${err.message}`);
+    });
+
     return failure.sendFailed
       ? {
           code: ExportResultCode.FAILED,

src/metrics/transform.ts

@@ -35,7 +35,7 @@ import {
 } from './constants';
 
 /** Transforms a OpenTelemetry instrument type to a GCM MetricKind. */
-function transformMetricKind(metric: MetricData): MetricKind {
+function _transformMetricKind(metric: MetricData): MetricKind {
   switch (metric.dataPointType) {
     case DataPointType.SUM:
       return metric.isMonotonic ? MetricKind.CUMULATIVE : MetricKind.GAUGE;
@@ -52,15 +52,17 @@ function transformMetricKind(metric: MetricData): MetricKind {
 }
 
 /** Transforms resource to Google Cloud Monitoring monitored resource */
-function transformResource(labels: {[key: string]: string}): MonitoredResource {
+function _transformResource(labels: {
+  [key: string]: string;
+}): MonitoredResource {
   return {
     type: SPANNER_RESOURCE_TYPE,
     labels: labels,
   } as MonitoredResource;
 }
 
 /** Transforms a OpenTelemetry ValueType to a GCM ValueType. */
-function transformValueType(metric: MetricData): ValueType {
+function _transformValueType(metric: MetricData): ValueType {
   const {
     dataPointType,
     descriptor: {name},
@@ -88,7 +90,7 @@ export function transformResourceMetricToTimeSeriesArray({
 
   return (
     scopeMetrics
-      // Only keep those whose scope.name matches 'gax-nodejs'.
+      // Only keep those whose scope.name matches 'spanner-nodejs'.
       .filter(({scope: {name}}) => name === SPANNER_METER_NAME)
       // Takes each metric array and flattens it into one array
       .flatMap(({metrics}) =>
@@ -98,15 +100,15 @@ export function transformResourceMetricToTimeSeriesArray({
       // Flatmap the data points in each metric to create a TimeSeries for each point
       .flatMap(metric =>
         metric.dataPoints.flatMap(dataPoint =>
-          createTimeSeries(metric, dataPoint)
+          _createTimeSeries(metric, dataPoint)
         )
       )
   );
 }
 /**
  * Creates a GCM TimeSeries.
  */
-function createTimeSeries<T>(
+function _createTimeSeries<T>(
   metric: MetricData,
   dataPoint: DataPoint<T>
 ): monitoring_v3.Schema$TimeSeries {
@@ -120,27 +122,27 @@ function createTimeSeries<T>(
 
   return {
     metric: transformedMetric,
-    resource: transformResource(monitoredResourceLabels),
-    metricKind: transformMetricKind(metric),
-    valueType: transformValueType(metric),
-    points: [transformPoint(metric, dataPoint)],
+    resource: _transformResource(monitoredResourceLabels),
+    metricKind: _transformMetricKind(metric),
+    valueType: _transformValueType(metric),
+    points: [_transformPoint(metric, dataPoint)],
     unit: metric.descriptor.unit,
   };
 }
 
 /**
  * Transform timeseries's point, so that metric can be uploaded to GCM.
  */
-function transformPoint<T>(
+function _transformPoint<T>(
   metric: MetricData,
   dataPoint: DataPoint<T>
 ): monitoring_v3.Schema$Point {
   switch (metric.dataPointType) {
     case DataPointType.SUM:
     case DataPointType.GAUGE:
       return {
-        value: transformNumberValue(
-          transformValueType(metric),
+        value: _transformNumberValue(
+          _transformValueType(metric),
           dataPoint.value as number
         ),
         interval: {
@@ -155,15 +157,15 @@ function transformPoint<T>(
       };
     case DataPointType.HISTOGRAM:
       return {
-        value: transformHistogramValue(dataPoint.value as Histogram),
+        value: _transformHistogramValue(dataPoint.value as Histogram),
         interval: {
           startTime: new PreciseDate(dataPoint.startTime).toISOString(),
           endTime: new PreciseDate(dataPoint.endTime).toISOString(),
         },
       };
     case DataPointType.EXPONENTIAL_HISTOGRAM:
       return {
-        value: transformExponentialHistogramValue(
+        value: _transformExponentialHistogramValue(
           dataPoint.value as ExponentialHistogram
         ),
         interval: {
@@ -215,7 +217,7 @@ function _normalizeLabelKey(key: string): string {
 }
 
 /** Transforms a OpenTelemetry Point's value to a GCM Point value. */
-function transformNumberValue(
+function _transformNumberValue(
   valueType: ValueType,
   value: number
 ): monitoring_v3.Schema$TypedValue {
@@ -227,7 +229,7 @@ function transformNumberValue(
   throw Error(`unsupported value type: ${valueType}`);
 }
 
-function transformHistogramValue(
+function _transformHistogramValue(
   value: Histogram
 ): monitoring_v3.Schema$TypedValue {
   return {
@@ -243,7 +245,7 @@ function transformHistogramValue(
   };
 }
 
-function transformExponentialHistogramValue(
+function _transformExponentialHistogramValue(
   value: ExponentialHistogram
 ): monitoring_v3.Schema$TypedValue {
   // Adapated from reference impl in Go which has more explanatory comments
@@ -297,10 +299,10 @@ function exhaust(switchValue: never) {
 
 export const _TEST_ONLY = {
   _normalizeLabelKey,
-  transformMetricKind,
+  _transformMetricKind,
   _extractLabels,
-  transformResource,
-  transformPoint,
-  transformValueType,
+  _transformResource,
+  _transformPoint,
+  _transformValueType,
   transformResourceMetricToTimeSeriesArray,
 };

test/metrics/spanner-metrics-exporter.test.ts

@@ -15,6 +15,7 @@
 import * as assert from 'assert';
 import * as sinon from 'sinon';
 import {MeterProvider, MetricReader} from '@opentelemetry/sdk-metrics';
+import {GoogleAuth} from 'google-auth-library';
 import {CloudMonitoringMetricsExporter} from '../../src/metrics/spanner-metrics-exporter';
 import {
   SPANNER_METER_NAME,
@@ -32,23 +33,24 @@ const PROJECT_ID = 'test-project';
 const INSTANCE_ID = 'test_instance';
 const DATABASE_ID = 'test_db';
 
+const auth = new GoogleAuth();
+
 // Ensure custom exporter is valid
 describe('CustomExporter', () => {
   it('should construct an exporter', () => {
-    const exporter = new CloudMonitoringMetricsExporter();
+    const exporter = new CloudMonitoringMetricsExporter({auth});
     assert.ok(typeof exporter.export === 'function');
     assert.ok(typeof exporter.shutdown === 'function');
   });
 
   it('should construct an exporter with credentials', () => {
-    const exporter = new CloudMonitoringMetricsExporter({
-      authOptions: {
-        credentials: {
-          client_email: 'noreply@fake.example.com',
-          private_key: 'this is a key',
-        },
+    const auth = new GoogleAuth({
+      credentials: {
+        client_email: 'fake',
+        private_key: '',
       },
     });
+    const exporter = new CloudMonitoringMetricsExporter({auth});
 
     assert(exporter);
     return (exporter['_projectId'] as Promise<string>).then(id => {
@@ -57,12 +59,12 @@ describe('CustomExporter', () => {
   });
 
   it('should be able to shutdown', async () => {
-    const exporter = new CloudMonitoringMetricsExporter();
+    const exporter = new CloudMonitoringMetricsExporter({auth});
     await assert.doesNotReject(exporter.shutdown());
   });
 
   it('should be able to force flush', async () => {
-    const exporter = new CloudMonitoringMetricsExporter();
+    const exporter = new CloudMonitoringMetricsExporter({auth});
     await assert.doesNotReject(exporter.forceFlush());
   });
 });
@@ -102,8 +104,6 @@ describe('Export', () => {
       database: DATABASE_ID,
       method: 'test_method',
       status: 'test_status',
-      directpath_enabled: 'true',
-      directpath_used: 'false',
       other: 'ignored',
     };
 
@@ -140,7 +140,7 @@ describe('Export', () => {
       unit: 'ms',
     });
 
-    exporter = new CloudMonitoringMetricsExporter();
+    exporter = new CloudMonitoringMetricsExporter({auth});
   });
 
   it('should export GCM metrics', async () => {