Skip to content

Commit a3e0bf4

Browse files
javiermolinarjaviermolinar
authored
fix: handle collisions with job and instance labels when targetInfo is enabled (#5780)
* fix: handle collisions with job and instance labels when targetInfo is enabled * changelog * refactor makeTraces method * nosec
1 parent b02dbb4 commit a3e0bf4

File tree

6 files changed

+68
-23
lines changed

6 files changed

+68
-23
lines changed

‎CHANGELOG.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@
1717
* [BUGFIX] Fix compactor to properly consider SSE-KMS information during metadata copy [#5774](https://github.com/grafana/tempo/pull/5774) (@steffsas)
1818
* [BUGFIX] Correctly track and reject too large traces in live stores. [#5757](https://github.com/grafana/tempo/pull/5757) (@joe-elliott)
1919
* [BUGFIX] Fix issues related to integer dedicated columns in vParquet5-preview2 [#5716](https://github.com/grafana/tempo/pull/5716) (@stoewer)
20+
* [BUGFIX] Fix: handle collisions with job and instance labels when targetInfo is enabled [#5774](https://github.com/grafana/tempo/pull/5774) (@javiermolinar)
2021
* [FEATURE] Added validation mode and tests for tempo-vulture [#5605](https://github.com/grafana/tempo/pull/5605)
2122

2223
# v2.9.0

‎modules/generator/processor/spanmetrics/config.go‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,10 @@ const (
2222
dimInstance = "instance"
2323
)
2424

25-
var intrinsicLabels = []string{dimService, dimSpanName, dimSpanKind, dimStatusCode, dimStatusMessage}
25+
var (
26+
intrinsicLabels = []string{dimService, dimSpanName, dimSpanKind, dimStatusCode, dimStatusMessage}
27+
targetInfoIntrinsicLabels = append(intrinsicLabels, dimJob, dimInstance)
28+
)
2629

2730
type Config struct {
2831
// Buckets for latency histogram in seconds.

‎modules/generator/processor/spanmetrics/spanmetrics.go‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -73,11 +73,11 @@ func New(cfg Config, reg registry.Registry, filteredSpansCounter, invalidUTF8Cou
7373
c := reclaimable.New(strutil.SanitizeLabelName, 10000)
7474

7575
for _, d := range cfg.Dimensions {
76-
labels = append(labels, SanitizeLabelNameWithCollisions(d, intrinsicLabels, c.Get))
76+
labels = append(labels, sanitizeLabelNameWithCollisions(d, intrinsicLabels, c.Get))
7777
}
7878

7979
for _, m := range cfg.DimensionMappings {
80-
labels = append(labels, SanitizeLabelNameWithCollisions(m.Name, intrinsicLabels, c.Get))
80+
labels = append(labels, sanitizeLabelNameWithCollisions(m.Name, intrinsicLabels, c.Get))
8181
}
8282

8383
err := validateUTF8LabelValues(labels)
@@ -135,7 +135,7 @@ func (p *Processor) aggregateMetrics(resourceSpans []*v1_trace.ResourceSpans) {
135135
jobName := processor_util.GetJobValue(rs.Resource.Attributes)
136136
instanceID, _ := processor_util.FindInstanceID(rs.Resource.Attributes)
137137
if p.Cfg.EnableTargetInfo {
138-
GetTargetInfoAttributesValues(&resourceLabels, &resourceValues, rs.Resource.Attributes, p.Cfg.TargetInfoExcludedDimensions, intrinsicLabels, p.sanitizeCache.Get)
138+
getTargetInfoAttributesValues(&resourceLabels, &resourceValues, rs.Resource.Attributes, p.Cfg.TargetInfoExcludedDimensions, p.sanitizeCache.Get)
139139
}
140140
for _, ils := range rs.ScopeSpans {
141141
for _, span := range ils.Spans {
@@ -270,7 +270,7 @@ func validateUTF8LabelValues(v []string) error {
270270
return nil
271271
}
272272

273-
func GetTargetInfoAttributesValues(keys, values *[]string, attributes []*v1_common.KeyValue, exclude, intrinsicLabels []string, sanitizeFn sanitizeFn) {
273+
func getTargetInfoAttributesValues(keys, values *[]string, attributes []*v1_common.KeyValue, exclude []string, sanitizeFn sanitizeFn) {
274274
// TODO allocate with known length, or take new params for existing buffers
275275
*keys = (*keys)[:0]
276276
*values = (*values)[:0]
@@ -284,14 +284,14 @@ func GetTargetInfoAttributesValues(keys, values *[]string, attributes []*v1_comm
284284
continue
285285
}
286286
if key != "service.name" && key != "service.namespace" && key != "service.instance.id" && !slices.Contains(exclude, key) {
287-
*keys = append(*keys, SanitizeLabelNameWithCollisions(key, intrinsicLabels, sanitizeFn))
287+
*keys = append(*keys, sanitizeLabelNameWithCollisions(key, targetInfoIntrinsicLabels, sanitizeFn))
288288
value := tempo_util.StringifyAnyValue(attrs.Value)
289289
*values = append(*values, value)
290290
}
291291
}
292292
}
293293

294-
func SanitizeLabelNameWithCollisions(name string, dimensions []string, sansanitizeFn sanitizeFn) string {
294+
func sanitizeLabelNameWithCollisions(name string, dimensions []string, sansanitizeFn sanitizeFn) string {
295295
sanitized := sansanitizeFn(name)
296296

297297
// check if same label as intrinsics

‎modules/generator/processor/spanmetrics/spanmetrics_test.go‎

Lines changed: 36 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -79,13 +79,39 @@ func TestSpanMetricsTargetInfoEnabled(t *testing.T) {
7979
cfg.RegisterFlagsAndApplyDefaults("", nil)
8080
cfg.HistogramBuckets = []float64{0.5, 1}
8181
cfg.EnableTargetInfo = true
82+
cfg.TargetInfoExcludedDimensions = []string{"random.res.attr"}
8283

8384
p, err := New(cfg, testRegistry, filteredSpansCounter, invalidUTF8SpanLabelsCounter)
8485
require.NoError(t, err)
8586
defer p.Shutdown(context.Background())
8687

8788
// TODO give these spans some duration so we can verify latencies are recorded correctly, in fact we should also test with various span names etc.
88-
batch := test.MakeBatch(10, nil)
89+
batch := test.MakeBatchWithAttributes(10, nil, []*common_v1.KeyValue{
90+
{
91+
Key: "job",
92+
Value: &common_v1.AnyValue{
93+
Value: &common_v1.AnyValue_StringValue{
94+
StringValue: "dummy-job",
95+
},
96+
},
97+
},
98+
{
99+
Key: "service.instance.id",
100+
Value: &common_v1.AnyValue{
101+
Value: &common_v1.AnyValue_StringValue{
102+
StringValue: "instance",
103+
},
104+
},
105+
},
106+
{
107+
Key: "instance",
108+
Value: &common_v1.AnyValue{
109+
Value: &common_v1.AnyValue_StringValue{
110+
StringValue: "dummy-instance",
111+
},
112+
},
113+
},
114+
})
89115

90116
p.PushSpans(context.Background(), &tempopb.PushSpansRequest{Batches: []*trace_v1.ResourceSpans{batch}})
91117

@@ -97,15 +123,23 @@ func TestSpanMetricsTargetInfoEnabled(t *testing.T) {
97123
"span_kind": "SPAN_KIND_CLIENT",
98124
"status_code": "STATUS_CODE_OK",
99125
"job": "test-service",
126+
"instance": "instance",
100127
})
101128

102129
assert.Equal(t, 10.0, testRegistry.Query("traces_spanmetrics_calls_total", lbls))
103-
104130
assert.Equal(t, 0.0, testRegistry.Query("traces_spanmetrics_latency_bucket", withLe(lbls, 0.5)))
105131
assert.Equal(t, 10.0, testRegistry.Query("traces_spanmetrics_latency_bucket", withLe(lbls, 1)))
106132
assert.Equal(t, 10.0, testRegistry.Query("traces_spanmetrics_latency_bucket", withLe(lbls, math.Inf(1))))
107133
assert.Equal(t, 10.0, testRegistry.Query("traces_spanmetrics_latency_count", lbls))
108134
assert.Equal(t, 10.0, testRegistry.Query("traces_spanmetrics_latency_sum", lbls))
135+
136+
targetInfoLabels := labels.FromMap(map[string]string{
137+
"job": "test-service",
138+
"__job": "dummy-job",
139+
"instance": "instance",
140+
"__instance": "dummy-instance",
141+
})
142+
assert.Equal(t, 1.0, testRegistry.Query("traces_target_info", targetInfoLabels))
109143
}
110144

111145
func TestSpanMetrics_dimensions(t *testing.T) {

‎modules/generator/processor/util/util.go‎

Lines changed: 5 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -56,14 +56,13 @@ func GetSpanMultiplier(ratioKey string, span *v1.Span, rs *v1_resource.Resource)
5656

5757
func GetJobValue(attributes []*v1_common.KeyValue) string {
5858
svName, _ := FindServiceName(attributes)
59-
namespace, _ := FindServiceNamespace(attributes)
60-
6159
// if service name is not present, consider job value empty
6260
if svName == "" {
6361
return ""
64-
} else if namespace != "" {
65-
namespace += "/"
6662
}
67-
68-
return namespace + svName
63+
namespace, _ := FindServiceNamespace(attributes)
64+
if namespace == "" {
65+
return svName
66+
}
67+
return namespace + "/" + svName
6968
}

‎pkg/util/test/req.go‎

Lines changed: 16 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,7 @@ func MakeSpanWithTimeWindow(traceID []byte, startTime uint64, endTime uint64) *v
4141

4242
func makeSpanWithAttributeCount(traceID []byte, count int, startTime uint64, endTime uint64) *v1_trace.Span {
4343
attributes := make([]*v1_common.KeyValue, 0, count)
44-
for i := 0; i < count; i++ {
44+
for range count {
4545
attributes = append(attributes, &v1_common.KeyValue{
4646
Key: RandomString(),
4747
Value: &v1_common.AnyValue{Value: &v1_common.AnyValue_StringValue{StringValue: RandomString()}},
@@ -122,15 +122,19 @@ func makeSpanWithAttributeCount(traceID []byte, count int, startTime uint64, end
122122
}
123123

124124
func MakeBatch(spans int, traceID []byte) *v1_trace.ResourceSpans {
125-
return makeBatchWithTimeRange(spans, traceID, nil)
125+
return makeBatchWithTimeRange(spans, traceID, nil, nil)
126+
}
127+
128+
func MakeBatchWithAttributes(spans int, traceID []byte, resAttributes []*v1_common.KeyValue) *v1_trace.ResourceSpans {
129+
return makeBatchWithTimeRange(spans, traceID, nil, resAttributes)
126130
}
127131

128132
type batchTimeRange struct {
129133
start uint64
130134
end uint64
131135
}
132136

133-
func makeBatchWithTimeRange(spans int, traceID []byte, timeRange *batchTimeRange) *v1_trace.ResourceSpans {
137+
func makeBatchWithTimeRange(spans int, traceID []byte, timeRange *batchTimeRange, resAttributes []*v1_common.KeyValue) *v1_trace.ResourceSpans {
134138
traceID = ValidTraceID(traceID)
135139

136140
batch := &v1_trace.ResourceSpans{
@@ -156,12 +160,16 @@ func makeBatchWithTimeRange(spans int, traceID []byte, timeRange *batchTimeRange
156160
},
157161
}
158162

163+
if resAttributes != nil {
164+
batch.Resource.Attributes = append(batch.Resource.Attributes, resAttributes...)
165+
}
166+
159167
var (
160168
ss *v1_trace.ScopeSpans
161169
ssCount int
162170
)
163171

164-
for i := 0; i < spans; i++ {
172+
for range spans {
165173
// occasionally make a new ss
166174
if ss == nil || rand.Int()%3 == 0 {
167175
ssCount++
@@ -191,7 +199,7 @@ func MakeTrace(requests int, traceID []byte) *tempopb.Trace {
191199
ResourceSpans: make([]*v1_trace.ResourceSpans, 0),
192200
}
193201

194-
for i := 0; i < requests; i++ {
202+
for range requests {
195203
trace.ResourceSpans = append(trace.ResourceSpans, MakeBatch(rand.Int()%20+1, traceID))
196204
}
197205

@@ -205,9 +213,9 @@ func MakeTraceWithTimeRange(requests int, traceID []byte, startTime, endTime uin
205213
ResourceSpans: make([]*v1_trace.ResourceSpans, 0),
206214
}
207215

208-
for i := 0; i < requests; i++ {
216+
for range requests {
209217
timeRange := &batchTimeRange{start: startTime, end: endTime}
210-
trace.ResourceSpans = append(trace.ResourceSpans, makeBatchWithTimeRange(rand.Int()%20+1, traceID, timeRange))
218+
trace.ResourceSpans = append(trace.ResourceSpans, makeBatchWithTimeRange(rand.Int()%20+1, traceID, timeRange, nil)) // #nosec G40
211219
}
212220

213221
return trace
@@ -218,7 +226,7 @@ func MakeTraceWithSpanCount(requests int, spansEach int, traceID []byte) *tempop
218226
ResourceSpans: make([]*v1_trace.ResourceSpans, 0),
219227
}
220228

221-
for i := 0; i < requests; i++ {
229+
for range requests {
222230
trace.ResourceSpans = append(trace.ResourceSpans, MakeBatch(spansEach, traceID))
223231
}
224232

0 commit comments

Comments
 (0)