Add signaltometrics connector to produce metrics from all signals #169
Conversation
There was a problem hiding this comment.
Thanks, I think this is looking great.
So just to make sure I understand: summary and histogram metrics have a default count of either 1 (log records & datapoints) or adjusted count (spans)? And then users can override this if they wish. I think that is sensible, just double checking my understanding.
Then where we currently aggregate a total span/transaction duration & count, we would create two metric definitions: one that sums up the durations, and one that sums up the adjusted counts. I think for the latter we'll need to either expose a new OTTL path for the adjusted count, or add a new OTTL converter, as discussed on Slack.
connector/signaltometrics/testdata/traces/exponential_histograms/config.yaml
Outdated
Show resolved
Hide resolved
| # Signal to metrics connector | ||
|
|
||
| WIP |
There was a problem hiding this comment.
[For reviewers] I will update this in a follow-up after all the configuration changes are in.
| } | ||
|
|
||
| // NewAggregator creates a new instance of aggregator. | ||
| func NewAggregator[K any](metrics pmetric.Metrics) *Aggregator[K] { |
There was a problem hiding this comment.
[For reviewers] This is currently missing unit-tests and is only tested via the connector tests, I will add these as a follow-up.
|
|
||
| func TestConnectorWithMetrics(t *testing.T) { | ||
| testCases := []string{ | ||
| "sum", |
There was a problem hiding this comment.
[For reviewers] I haven't added tests for histograms for the metric to metric case. This is because, I couldn't think of a good example for these without implementing conditions (will be a follow-up). Use-cases like gauge to histograms OR histograms to exponential histograms will only make sense if we can add conditions on metric types or metric names. So, these will be added with the PR for conditions.
connector/signaltometricsconnector/testdata/logs/exponential_histograms/config.yaml
Outdated
Show resolved
Hide resolved
| // MetricInfo for a data type | ||
| type MetricInfo struct { |
There was a problem hiding this comment.
Perhaps "MetricDefinition" would be a more informative name?
There was a problem hiding this comment.
I use MetricDef for the parsed config (in the model/*), maybe we can use MetricConfig here? WDYT?
There was a problem hiding this comment.
Does it matter if they have the same name? I think it's still a metric definition from the user's perspective. I wouldn't go with MetricConfig, since it's in the config package.
Anyway, not critical that we address this atm, we can come back to it.
connector/signaltometricsconnector/internal/aggregator/aggregator.go
Outdated
Show resolved
Hide resolved
connector/signaltometricsconnector/internal/aggregator/aggregator.go
Outdated
Show resolved
Hide resolved
connector/signaltometricsconnector/internal/aggregator/aggregator.go
Outdated
Show resolved
Hide resolved
connector/signaltometricsconnector/internal/aggregator/aggregator.go
Outdated
Show resolved
Hide resolved
|
@axw @felixbarny Addressed the concerns, for some of them I have created issues. Should be ready for another pass. |
signaltometricsis built as a connector that is capable of producing metrics from all signal types. The component utilizes OTTL to allow configuring the data and the metric types used to generate the metrics.Related to: https://github.com/elastic/opentelemetry-dev/issues/372