-
Notifications
You must be signed in to change notification settings - Fork 152
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
Use pdata for prometheus monitor #5545
Conversation
4eb0309
to
7cc3c74
Compare
7cc3c74
to
6b0d302
Compare
metric := pmetric.NewMetric() | ||
metric.SetName(name + "_count") | ||
sum := metric.SetEmptySum() | ||
sum.SetIsMonotonic(true) | ||
sum.SetAggregationTemporality(pmetric.AggregationTemporalityCumulative) | ||
dp := sum.DataPoints().AppendEmpty() | ||
dp.SetIntValue(int64(h.GetSampleCount())) //nolint:gosec // disable G115 | ||
_ = dp.Attributes().FromRaw(dims) | ||
dps = append(dps, metric) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, we are creating one metric with one data point for each Prometheus time series. Is that correct? I don't see any combining logic down the road...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like the existing sfxDatapointsToPDataMetrics
implementation does pretty much the same though...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this is a simple follow through to make sure we get the same behavior for now.
Do we have any tests to confirm that the output hasn't changed? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM assuming we have tests covering the output
Yes, we have multiple tests that check with golden files that the prometheus metrics match. Both for simple gauges and counters and histograms. |
No description provided.