Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Consider adding summary support to metricdata #4587

Closed
dashpole opened this issue Oct 5, 2023 · 3 comments · Fixed by #4622
Closed

Consider adding summary support to metricdata #4587

dashpole opened this issue Oct 5, 2023 · 3 comments · Fixed by #4622
Assignees
Labels
enhancement New feature or request pkg:bridges Related to a bridge package
Milestone

Comments

@dashpole
Copy link
Contributor

dashpole commented Oct 5, 2023

#4571 is currently blocked on a spec decision. In the meantime, we can at least look at what adding summary support would look like in the Go SDK with a prototype.

@dashpole dashpole added the enhancement New feature or request label Oct 5, 2023
@dashpole dashpole added the pkg:bridges Related to a bridge package label Oct 5, 2023
@dashpole dashpole self-assigned this Oct 9, 2023
@dashpole dashpole moved this from Todo to In Progress in Go: Metric OpenCensus Bridge (GA) Oct 9, 2023
@dashpole
Copy link
Contributor Author

dashpole commented Oct 9, 2023

See #4600 for what adding the summary type to the data model and bridge would look like.

It does not require introducing the Summary type in the SDK (e.g. through views), since the Aggregation used in the view's Stream differs from metricdata.Aggregation.

@dashpole
Copy link
Contributor Author

dashpole commented Oct 9, 2023

Current exporters, have switch statements over Data.(type) of the aggregation, and either error on unknown values, or ignore them. So this would be a relatively safe change. We would want to follow-up by introducing summary support to OTLP and Prometheus exporters.

switch v := m.Data.(type) {
case metricdata.Histogram[int64]:
addHistogramMetric(ch, v, m, keys, values, name)
case metricdata.Histogram[float64]:
addHistogramMetric(ch, v, m, keys, values, name)
case metricdata.Sum[int64]:
addSumMetric(ch, v, m, keys, values, name)
case metricdata.Sum[float64]:
addSumMetric(ch, v, m, keys, values, name)
case metricdata.Gauge[int64]:
addGaugeMetric(ch, v, m, keys, values, name)
case metricdata.Gauge[float64]:
addGaugeMetric(ch, v, m, keys, values, name)
}

switch a := m.Data.(type) {
case metricdata.Gauge[int64]:
out.Data = Gauge[int64](a)
case metricdata.Gauge[float64]:
out.Data = Gauge[float64](a)
case metricdata.Sum[int64]:
out.Data, err = Sum[int64](a)
case metricdata.Sum[float64]:
out.Data, err = Sum[float64](a)
case metricdata.Histogram[int64]:
out.Data, err = Histogram(a)
case metricdata.Histogram[float64]:
out.Data, err = Histogram(a)
case metricdata.ExponentialHistogram[int64]:
out.Data, err = ExponentialHistogram(a)
case metricdata.ExponentialHistogram[float64]:
out.Data, err = ExponentialHistogram(a)
default:
return out, fmt.Errorf("%w: %T", errUnknownAggregation, a)
}

@dashpole
Copy link
Contributor Author

We discussed this at the SIG meeting yesterday, and are going to move forward with adding this to the metricdata package.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request pkg:bridges Related to a bridge package
Projects
No open projects
Development

Successfully merging a pull request may close this issue.

2 participants