Bump Tessera & switch to OpenTelemetry #242
Conversation
AlCutter
added
enhancement
|
|
||
| lastSCTTimestamp = mustCreate(meter.Int64Gauge("tesseract.last_sct.timestamp", | ||
| metric.WithDescription("Time of last SCT since epoch"), | ||
| metric.WithUnit("ms"))) |
There was a problem hiding this comment.
(To avoid ambiguity with my other comment, let's keep ms here since this is the unit in the RFC)
internal/scti/handlers.go
Outdated
| metric.WithDescription("Index of last SCT"), | ||
| metric.WithUnit("{entry}"))) | ||
|
|
||
| reqsCounter = mustCreate(meter.Int64Counter("tesseract.http_request.count", |
There was a problem hiding this comment.
I think the unit should be "request" here? https://opentelemetry.io/docs/specs/semconv/http/http-metrics/#metric-httpserveractive_requests
internal/scti/handlers.go
Outdated
| rspLatency = mustCreate(meter.Float64Histogram("tesseract.http_response.duration", | ||
| metric.WithDescription("CT HTTP response duration"), | ||
| metric.WithExplicitBucketBoundaries(otel.LatencyHistogramBuckets...), | ||
| metric.WithUnit("ms"))) |
There was a problem hiding this comment.
Let's use seconds here as a unit: it's Opentelemetry's default unit for time. I think we should also rename this metric to http_request.duration rather, to align with https://opentelemetry.io/docs/specs/semconv/http/http-metrics/#metric-httpserverrequestduration. I can't tell for sure, but should this be tesseract.http.request.count rather?
In all fairness: If I were to write this from scratch, I'd use latency... and if I could not use latency I would use "response.duration". But it looks like people who've thought about this for some time have made other decisions, let's stick to default and embrace Opentelemtry way of doing things.
| Name: "http_reqs", | ||
| Help: "Number of requests", | ||
| }, | ||
| []string{"origin", "ep"}) |
There was a problem hiding this comment.
Do I gather correctly that we can't specify attributes at metric creation time?
There was a problem hiding this comment.
This is a feature that's coming, but still "in development".
We should be able to come back and add such "advisory attributes" later: https://opentelemetry.io/docs/specs/otel/metrics/api/#instrument-advisory-parameter-attributes
internal/scti/otel.go
Outdated
| ) | ||
|
|
||
| var ( | ||
| codeKey = attribute.Key("tesseract.code") |
There was a problem hiding this comment.
I think this should be status_code from https://opentelemetry.io/docs/specs/semconv/http/http-metrics/
This reverts commit 96ce8e4.
This reverts commit 96ce8e4.
This reverts commit 96ce8e4. # Conflicts: # internal/scti/handlers.go
This reverts commit 96ce8e4. # Conflicts: # internal/scti/handlers.go # Conflicts: # go.mod
This PR bumps the
tesseradep to HEAD, and updates monitoring in this package to adopt OpenTelemetry as used in Tessera.Existing prometheus metric have been migrated to OTel, and a few initial top-level trace spans added to show how it works. More metrics & traces could be added as the code stablises.
The GCP binary has been updated to auto-export telemetry to Cloud Monitoring.
For now, I've removed the prometheus metrics exporting from the AWS binary and OTel export support will need to be added at a later date.