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

feat: add support for MetricTypeSum #37156

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

perebaj
Copy link
Contributor

@perebaj perebaj commented Jan 12, 2025

Description

Prometheus translation rw2 add support for MetricTypeSum.

The current work was inspired by #36353

Link to tracking issue #33661

@dashpole
Copy link
Contributor

Error: prometheusremotewrite/metrics_to_prw_v2.go:118:88: (*prometheusConverterV2).addSample - result 0 (*github.com/prometheus/prometheus/prompb/io/prometheus/write/v2.TimeSeries) is never used (unparam)
func (c *prometheusConverterV2) addSample(sample *writev2.Sample, lbls []prompb.Label) *writev2.TimeSeries {

Copy link
Contributor

@dashpole dashpole left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm after lint errors are fixed

@perebaj
Copy link
Contributor Author

perebaj commented Jan 18, 2025

I’m encountering inconsistent behavior with the TestFromMetricsV2 test case. It sometimes passes but fails at other times, even though I haven’t made any changes related to its functionality. I’m trying to understand what’s causing this issue...

@perebaj
Copy link
Contributor Author

perebaj commented Jan 23, 2025

I finally identified the issue:

The test function func TestFromMetricsV2(t *testing.T) is executed twice. During the first execution, it processes a metric of type Sum (monotonic) and calls the addGaugeNumberDataPoints function. In the second execution, it processes a gauge metric, which also calls addGaugeNumberDataPoints.

When the addSample function is invoked, it relies on the c.symbolTable (line 120 below). This function assigns a numerical reference to each individual label.

For example, consider the following labels:

"series_name_1", "value-1", "series_name_2", "value-2", "__name__", "sum_1", "gauge_1"

First Execution
If the label order is:

series_name_1, value-1, __name__, sum_1, series_name_2, value-2

The corresponding map will look like this:

{
    series_name_1: 1,
    value-1: 2,
    __name__: 3,
    sum_1: 4,
    series_name_2: 5,
    value-2: 6
}

Thus, the LabelsRefs generated will be:

1, 2, 3, 4, 5, 6

Second Execution

Now, if the labels arrive in a different order, such as:

__name__, gauge_1

The updated map will become:

{
    series_name_1: 1,
    value-1: 2,
    __name__: 3,
    sum_1: 4,
    series_name_2: 5,
    value-2: 6,
    gauge_1: 9
}

the final LabelsRefs will be:

3, 9

The issue with the current approach is that the outcome of the second execution is directly influenced by the order of labels processed in the first execution. The simplest way that I found to address this, was sorting the labels before assigning the numerical reference. Is that makes sense?

func (c *prometheusConverterV2) addSample(sample *writev2.Sample, lbls []prompb.Label) *writev2.TimeSeries {
if sample == nil || len(lbls) == 0 {
// This shouldn't happen
return nil
}
buf := make([]uint32, 0, len(lbls)*2)
var off uint32
for _, l := range lbls {
off = c.symbolTable.Symbolize(l.Name)
buf = append(buf, off)
off = c.symbolTable.Symbolize(l.Value)
buf = append(buf, off)
}
ts := writev2.TimeSeries{
LabelsRefs: buf,
Samples: []writev2.Sample{*sample},
}
c.unique[timeSeriesSignature(lbls)] = &ts
return &ts
}

@dashpole
Copy link
Contributor

Should the labels just be sorted before comparing in the test? If we aren't required to sort them for the protocol (or are we?), then we should probably avoid doing that?

@perebaj
Copy link
Contributor Author

perebaj commented Jan 23, 2025

Should the labels just be sorted before comparing in the test? If we aren't required to sort them for the protocol (or are we?), then we should probably avoid doing that?

Taking a deep look into the places where we sort labels. We are already doing it inside addGaugeNumberDataPoints

labels := createAttributes(
resource,
pt.Attributes(),
settings.ExternalLabels,
nil,
true,
model.MetricNameLabel,
name,
)

but we are doing that before add the __name__ label name in the slice.

What do you recommend in this case? I'm not sure if it's possible to sort the labels before comparing in the test.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants