Skip to content

Commit

Permalink
prometheusremotewrite: Don't generate target_info unless at least o…
Browse files Browse the repository at this point in the history
…ne identifying label is defined (#32148)

**Description:** <Describe what has changed.>
I propose modifying the Prometheus remote write translator such that it
will only generate the `target_info` metric if at least one identifying
label is defined, i.e. either `job` or `instance` (or both). The
rationale is that without any identifying labels, `target_info` is
useless (you can't join against it).

See [Slack
thread](https://cloud-native.slack.com/archives/C01LSCJBXDZ/p1712161732017409)
for background.

**Testing:** <Describe what testing was performed and which tests were
added.>
I've modified the `addResourceTargetInfo` tests to reflect that at least
one identifying label is required, and added some new ones to cover the
different cases. I also modify a couple of
`prometheusremotewriteexporter` tests that are affected by this change,
as they don't define the identifying resource attributes.

**Documentation:** <Describe the documentation added.>

---------

Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>
Co-authored-by: Anthony Mirabella <a9@aneurysm9.com>
  • Loading branch information
aknuds1 and Aneurysm9 authored Apr 30, 2024
1 parent 06c970d commit 95e8f8f
Show file tree
Hide file tree
Showing 4 changed files with 80 additions and 40 deletions.
27 changes: 27 additions & 0 deletions .chloggen/bugfix_target-info-required-attrs.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
# Use this changelog template to create an entry for release notes.

# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix'
change_type: bug_fix

# The name of the component, or a single word describing the area of concern, (e.g. filelogreceiver)
component: prometheusremotewrite

# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`).
note: Change prometheusremotewrite.FromMetrics so that the target_info metric is only generated if at least one identifying OTel resource attribute (service.name and/or service.instance.id) is defined.

# Mandatory: One or more tracking issues related to the change. You can use the PR number here if no issue exists.
issues: [32148]

# (Optional) One or more lines of additional information to render under the primary note.
# These lines will be padded with 2 spaces and then inserted directly into the document.
# Use pipe (|) for multiline entries.
subtext:

# If your change doesn't affect end users or the exported elements of any package,
# you should instead start your pull request title with [chore] or use the "Skip Changelog" label.
# Optional: The change log or logs in which this entry should be included.
# e.g. '[user]' or '[user, api]'
# Include 'user' if the change is relevant to end users.
# Include 'api' if there is a change to a library API.
# Default: '[user]'
change_logs: [user]
2 changes: 1 addition & 1 deletion exporter/prometheusremotewriteexporter/exporter_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -497,7 +497,7 @@ func Test_PushMetrics(t *testing.T) {
name: "intSum_case",
metrics: intSumBatch,
reqTestFunc: checkFunc,
expectedTimeSeries: 5,
expectedTimeSeries: 4,
httpResponseCode: http.StatusAccepted,
},
{
Expand Down
13 changes: 13 additions & 0 deletions pkg/translator/prometheusremotewrite/helper.go
Original file line number Diff line number Diff line change
Expand Up @@ -540,6 +540,19 @@ func addResourceTargetInfo(resource pcommon.Resource, settings Settings, timesta
}

labels := createAttributes(resource, attributes, settings.ExternalLabels, identifyingAttrs, false, model.MetricNameLabel, name)
haveIdentifier := false
for _, l := range labels {
if l.Name == model.JobLabel || l.Name == model.InstanceLabel {
haveIdentifier = true
break
}
}

if !haveIdentifier {
// We need at least one identifying label to generate target_info.
return
}

sample := &prompb.Sample{
Value: float64(1),
// convert ns to ms
Expand Down
78 changes: 39 additions & 39 deletions pkg/translator/prometheusremotewrite/helper_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@
package prometheusremotewrite

import (
"fmt"
"math"
"sort"
"testing"
Expand Down Expand Up @@ -532,10 +531,18 @@ func TestAddResourceTargetInfo(t *testing.T) {
conventions.AttributeServiceInstanceID: "service-instance-id",
}
resourceWithServiceAttrs := pcommon.NewResource()
assert.NoError(t, resourceWithServiceAttrs.Attributes().FromRaw(resourceAttrMap))
require.NoError(t, resourceWithServiceAttrs.Attributes().FromRaw(resourceAttrMap))
resourceWithServiceAttrs.Attributes().PutStr("resource_attr", "resource-attr-val-1")
resourceWithOnlyServiceAttrs := pcommon.NewResource()
assert.NoError(t, resourceWithOnlyServiceAttrs.Attributes().FromRaw(resourceAttrMap))
require.NoError(t, resourceWithOnlyServiceAttrs.Attributes().FromRaw(resourceAttrMap))
// service.name is an identifying resource attribute.
resourceWithOnlyServiceName := pcommon.NewResource()
resourceWithOnlyServiceName.Attributes().PutStr(conventions.AttributeServiceName, "service-name")
resourceWithOnlyServiceName.Attributes().PutStr("resource_attr", "resource-attr-val-1")
// service.instance.id is an identifying resource attribute.
resourceWithOnlyServiceID := pcommon.NewResource()
resourceWithOnlyServiceID.Attributes().PutStr(conventions.AttributeServiceInstanceID, "service-instance-id")
resourceWithOnlyServiceID.Attributes().PutStr("resource_attr", "resource-attr-val-1")
for _, tc := range []struct {
desc string
resource pcommon.Resource
Expand All @@ -549,61 +556,54 @@ func TestAddResourceTargetInfo(t *testing.T) {
},
{
desc: "disable target info metric",
resource: testdata.GenerateMetricsNoLibraries().ResourceMetrics().At(0).Resource(),
resource: resourceWithOnlyServiceName,
settings: Settings{DisableTargetInfo: true},
},
{
desc: "with resource",
desc: "with resource missing both service.name and service.instance.id resource attributes",
resource: testdata.GenerateMetricsNoLibraries().ResourceMetrics().At(0).Resource(),
timestamp: testdata.TestMetricStartTimestamp,
},
{
desc: "with resource including service.instance.id, and missing service.name resource attribute",
resource: resourceWithOnlyServiceID,
timestamp: testdata.TestMetricStartTimestamp,
wantLabels: []prompb.Label{
{
Name: model.MetricNameLabel,
Value: targetMetricName,
},
{
Name: "resource_attr",
Value: "resource-attr-val-1",
},
{Name: model.MetricNameLabel, Value: "target_info"},
{Name: model.InstanceLabel, Value: "service-instance-id"},
{Name: "resource_attr", Value: "resource-attr-val-1"},
},
},
{
desc: "with resource, with namespace",
resource: testdata.GenerateMetricsNoLibraries().ResourceMetrics().At(0).Resource(),
desc: "with resource including service.name, and missing service.instance.id resource attribute",
resource: resourceWithOnlyServiceName,
timestamp: testdata.TestMetricStartTimestamp,
wantLabels: []prompb.Label{
{Name: model.MetricNameLabel, Value: "target_info"},
{Name: model.JobLabel, Value: "service-name"},
{Name: "resource_attr", Value: "resource-attr-val-1"},
},
},
{
desc: "with valid resource, with namespace",
resource: resourceWithOnlyServiceName,
timestamp: testdata.TestMetricStartTimestamp,
settings: Settings{Namespace: "foo"},
wantLabels: []prompb.Label{
{
Name: model.MetricNameLabel,
Value: fmt.Sprintf("foo_%s", targetMetricName),
},
{
Name: "resource_attr",
Value: "resource-attr-val-1",
},
{Name: model.MetricNameLabel, Value: "foo_target_info"},
{Name: model.JobLabel, Value: "service-name"},
{Name: "resource_attr", Value: "resource-attr-val-1"},
},
},
{
desc: "with resource, with service attributes",
resource: resourceWithServiceAttrs,
timestamp: testdata.TestMetricStartTimestamp,
wantLabels: []prompb.Label{
{
Name: model.MetricNameLabel,
Value: targetMetricName,
},
{
Name: "instance",
Value: "service-instance-id",
},
{
Name: "job",
Value: "service-namespace/service-name",
},
{
Name: "resource_attr",
Value: "resource-attr-val-1",
},
{Name: model.MetricNameLabel, Value: "target_info"},
{Name: model.InstanceLabel, Value: "service-instance-id"},
{Name: model.JobLabel, Value: "service-namespace/service-name"},
{Name: "resource_attr", Value: "resource-attr-val-1"},
},
},
{
Expand Down

0 comments on commit 95e8f8f

Please sign in to comment.