feat: otel metrics #819
feat: otel metrics #819
Conversation
Pull Request Test Coverage Report for Build 20783124376Details
💛 - Coveralls |
fenos
force-pushed
the
monitoring/otel-metrics
branch
from
ddcc147 to
c9d5fd0
Compare
| { | ||
| "datasource": { | ||
| "type": "prometheus", | ||
| "uid": "local_prometheus" | ||
| }, | ||
| "editorMode": "code", | ||
| "expr": "sum(increase(storage_api_otel_upload_started_total{region=~\"$region\", instance=~\"$instance\"}[$__range]))", | ||
| "hide": false, | ||
| "instant": false, | ||
| "legendFormat": "__auto", | ||
| "range": true, | ||
| "refId": "C" | ||
| } |
There was a problem hiding this comment.
This was supposed to be multipart I guess according to description but instead duplicates query A
| Context, | ||
| } from '@opentelemetry/api' | ||
| import { ReadableSpan, SpanProcessor } from '@opentelemetry/sdk-trace-base' | ||
| import { Span as SdkSpan } from '@opentelemetry/sdk-trace-base/build/src/Span' |
There was a problem hiding this comment.
Is it possible not to import from build?
| hostMetrics.start() | ||
|
|
||
| // Register Node.js runtime instrumentations | ||
| registerInstrumentations({ |
There was a problem hiding this comment.
I think we need to capture returned function unregister here to pass shutdown below. Otherwise, it might prevent process from exiting
| this.updateMetrics().catch(() => { | ||
| // Ignore errors | ||
| }) | ||
| this._updateInterval = setInterval(() => this.updateMetrics().catch(() => {}), 5000) |
There was a problem hiding this comment.
re:https://github.com/supabase/storage/pull/819/changes#r2686374454
If we address above comment, this would be unnecessary but I would add unref call here
| is_standard: uploadType === 'standard' ? 1 : 0, | ||
| is_s3: uploadType === 's3' ? 1 : 0, | ||
| fileUploadedSuccess.add(1, { | ||
| is_multipart: String(uploadType === 'resumable' ? 1 : 0), |
There was a problem hiding this comment.
There is an inconsistency in the label values of fileUploadStarted (https://github.com/supabase/storage/pull/819/changes#diff-27bcc650282e1d4d6ab25be5c1bf343e75aa41e76844d93be70a146b4391a82bR83). Its is_multipart can be true or false but here 1 or 0. Is this intentional?
What kind of change does this PR introduce?
Feature
What is the current behavior?
Currently metrics are gathered using the prom-client and exposed to a metric endpoint
What is the new behavior?
ENABLE_PROMETHEUS_METRICS)Grafana dashboard
Running completely locally