feat: Add plumbing PR for client side metrics to support the open telemetry instruments #1569
Conversation
for each call
…b.com/googleapis/nodejs-bigtable into 359913994-first-PR-add-infrastructure
|
For threads like this it is not letting me comment in the conversation tab |
Also add if statement for timeseries.
There was a problem hiding this comment.
This is looking very solid! My only remaining request would be to rename the Metrics interface to Instruments or MetricInstruments, since Metrics has a different meaning in OTel
After that, this should be good to merge
| }, | ||
| }, | ||
| ], | ||
| }); |
There was a problem hiding this comment.
does this one need a unit field, like for the distribution?
There was a problem hiding this comment.
The tests that export results to Google Cloud Monitoring succeed at exporting the metrics so I don't think so. If we did have a unit for connectivity_error_count and retry_count then what would it be? These are just counters.
| ], | ||
| labels: resourceLabels, | ||
| }, | ||
| valueType: 'INT64', |
There was a problem hiding this comment.
Right, this is just for isCounterValue, makes sense!
I'd still recommend refactoring this function a bit. Either breaking out counter/distribution into two helper methods, or a single helper if there are enough shared fields to combine them
But that's just a style suggestion
src/client-side-metrics/exporter.ts
Outdated
| // to 0 then the operation failed. Open telemetry logs errors to the | ||
| // console when the resultCallback passes in non-zero code values and | ||
| // logs nothing when the code is 0. | ||
| const exportResult = {code: 0}; |
There was a problem hiding this comment.
FWIW it is also used a couple times in MetricsExporter, so I still think this would be a good idea. But this is a small nit
| * A collection of OpenTelemetry metric instruments used to record | ||
| * Bigtable client-side metrics. | ||
| */ | ||
| interface Metrics { |
There was a problem hiding this comment.
This should probably be renamed to Instruments or MetricInstruments, since Metrics has a different meaning in OTel
| * which will be provided to the exporter in every export call. | ||
| * | ||
| */ | ||
| private getMetrics(projectId: string): Metrics { |
There was a problem hiding this comment.
Yup. Renamed to getInstruments.
| instanceId: data.metricsCollectorData.instanceId, | ||
| table: data.metricsCollectorData.table, | ||
| cluster: data.metricsCollectorData.cluster, | ||
| zone: data.metricsCollectorData.zone, |
serverTime and connectivityErrorCount should only be read once.
The optimization means some code gets skipped therefore lower latency in the tests
|
Re: #1569 (comment). I think this is the MetricsExporter file? We can change |
Summary:
In this PR we introduce all the plumbing that runs the operations from the point where the metrics handler receives data to the point where the data is sent to the Google Cloud Monitoring dashboard.
The diagram below shows all the pieces added in this PR in green:
A test stub exists that contains the requests that the metrics collector passes into its metrics handler when a fake method is used. This test stub is used to pass data into the metrics handler in this PR and test the new pipeline that this PR adds.
This is a follow up PR to #1566.
Changes:
package.json: Add the package that contains the MetricServiceClient used for sending data to Google Cloud Monitoring.src/client-side-metrics/exporter.ts: Add the CloudMonitoringExporter which accepts metrics and then converts them to the right format and sends them to the Google Cloud Monitoring dashboard.src/client-side-metrics/gcp-metrics-handler.ts: The metrics handler that stores requests in open telemetry instruments and then periodically sends them to its exporter.system-test/cloud-monitoring-exporter.ts: System tests that write to the google cloud monitoring dashboard with a fixed value passed into the exporter.system-test/gcp-metrics-handler.ts: A system test that passes fixed values into thegcp-metrics-handler.tsand ensures it properly writes information to the Google Cloud dashboard.test-common/expected-otel-export-input.ts: Some fixed constants related to values open telemetry provides to the exporter.test/metrics-collector/gcp-metrics-handler.ts: A test that ensures the GCP Metrics handler passes the right information to the exporter.test/metrics-collector/metrics-to-request.ts: A test that ensures the method that converts the value the exporter receives to the value that GCM needs is correct.Next Steps: