Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Pull request overview
Adds analytics instrumentation to the OTLP trace exporter path so span ingestion activity is visible in PostHog via the existing Analytics.Count aggregation mechanism.
Changes:
- Introduces a new analytics resource constant:
otel-span. - Wires an
analytics.Analyticsdependency into the OTel collector and emits aCountevent duringExport. - Passes the engine’s analytics instance into the OTel collector during engine startup (v0 and v1 configs).
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| pkg/analytics/analytics.go | Adds OtelSpan as a new analytics resource identifier. |
| internal/services/otelcol/server.go | Emits an analytics Count event during OTLP trace export. |
| internal/services/otelcol/otelcol.go | Adds WithAnalytics option and stores analytics instance on the collector implementation. |
| cmd/hatchet-engine/engine/run.go | Wires sc.Analytics into the OTel collector initialization for both v0/v1 engine configs. |
| oc.a.Count(ctx, analytics.OtelSpan, analytics.Create, analytics.Properties{ | ||
| "span_count": len(spans), | ||
| }) |
There was a problem hiding this comment.
Count aggregates by (resource, action, tenant, token, properties-hash) and increments by 1 per call (see pkg/analytics/posthog/posthog.go:154). Passing the raw span_count as a property will create very high-cardinality buckets (and may hit the aggregator maxKeys limit, dropping events), and it still won’t actually count spans (it counts export requests).
Consider either (a) emitting Count with low-cardinality props only (e.g. has_truncated, has_error) and relying on the built-in count field, or (b) adding an explicit “count N” API (or bucketizing span_count into a small set of ranges) if you need span-volume metrics. Also consider moving the analytics call after truncation and after CreateSpans/lookup-table writes succeed so it represents accepted spans, not just attempted exports.
There was a problem hiding this comment.
Kinda agree with the robot here about the high cardinality ☝️
Why are we storing span_count as a label/property of the metric? Am aware the interface doesn't account for this but should we not rather be adding the len(spans) to the count value i.e
// could make a private Property interface to ensure whatever props we pass in are valid i.e
type Property interface {
private()
}
type Properties map[string]interface{}
func (Properties) private() {}
type Increment int
func (Increment) private() {}
type Analytics interface {
// ...
Count(ctx context.Context, resource Resource, action Action, props ...Property)
// ...
}Then we can increment this like:
oc.a.Count(ctx, analytics.OtelSpan, analytics.Create, analytics.Increment(len(spans)))Otherwise, could always just do a loop without any props:
for len(spans) {
oc.a.Count(ctx, analytics.OtelSpan, analytics.Create)
}There was a problem hiding this comment.
yep! looks like the analytics.instructions.md file is working :)
There was a problem hiding this comment.
just removing it for now, we should at some point add the count to the interface
gregfurman
left a comment
There was a problem hiding this comment.
LGTM! Just a small nit about passing in an empty properties struct when we could just leave the param empty.
Co-authored-by: Greg Furman <31275503+gregfurman@users.noreply.github.com>
Description
Instruments analytics counting for span exporter.
Type of change