Skip to content

Commit 19547a5

Browse files
simonswinesimonswine
authored
feat: Align missing service_name behaviour with otel spec (#4038)
* feat: Align missing service_name behaviour with otel spec When looking at otel-ebpf-profiler I noticed we ignored their specification: https://github.com/open-telemetry/opentelemetry-go/blob/ecfb73581f1b05af85fc393c3ce996a90cf2a5e2/semconv/v1.30.0/attribute_group.go#L10011-L10025 * Fix integration test * More integration * Make more use of constants
1 parent db0463f commit 19547a5

File tree

6 files changed

+134
-23
lines changed

6 files changed

+134
-23
lines changed

‎pkg/distributor/distributor.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -291,7 +291,7 @@ func (d *Distributor) PushParsed(ctx context.Context, req *distributormodel.Push
291291
for _, series := range req.Series {
292292
serviceName := phlaremodel.Labels(series.Labels).Get(phlaremodel.LabelNameServiceName)
293293
if serviceName == "" {
294-
series.Labels = append(series.Labels, &typesv1.LabelPair{Name: phlaremodel.LabelNameServiceName, Value: "unspecified"})
294+
series.Labels = append(series.Labels, &typesv1.LabelPair{Name: phlaremodel.LabelNameServiceName, Value: phlaremodel.AttrServiceNameFallback})
295295
}
296296
sort.Sort(phlaremodel.Labels(series.Labels))
297297
}

‎pkg/distributor/distributor_test.go

Lines changed: 74 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -27,22 +27,20 @@ import (
2727
"github.com/stretchr/testify/assert"
2828
"github.com/stretchr/testify/require"
2929

30-
"github.com/grafana/pyroscope/pkg/distributor/ingest_limits"
31-
testhelper2 "github.com/grafana/pyroscope/pkg/pprof/testhelper"
32-
3330
profilev1 "github.com/grafana/pyroscope/api/gen/proto/go/google/v1"
34-
distributormodel "github.com/grafana/pyroscope/pkg/distributor/model"
35-
phlaremodel "github.com/grafana/pyroscope/pkg/model"
36-
pprof2 "github.com/grafana/pyroscope/pkg/pprof"
37-
"github.com/grafana/pyroscope/pkg/util"
38-
3931
pushv1 "github.com/grafana/pyroscope/api/gen/proto/go/push/v1"
4032
"github.com/grafana/pyroscope/api/gen/proto/go/push/v1/pushv1connect"
4133
typesv1 "github.com/grafana/pyroscope/api/gen/proto/go/types/v1"
4234
connectapi "github.com/grafana/pyroscope/pkg/api/connect"
4335
"github.com/grafana/pyroscope/pkg/clientpool"
36+
"github.com/grafana/pyroscope/pkg/distributor/ingest_limits"
37+
distributormodel "github.com/grafana/pyroscope/pkg/distributor/model"
38+
phlaremodel "github.com/grafana/pyroscope/pkg/model"
39+
pprof2 "github.com/grafana/pyroscope/pkg/pprof"
40+
pproftesthelper "github.com/grafana/pyroscope/pkg/pprof/testhelper"
4441
"github.com/grafana/pyroscope/pkg/tenant"
4542
"github.com/grafana/pyroscope/pkg/testhelper"
43+
"github.com/grafana/pyroscope/pkg/util"
4644
"github.com/grafana/pyroscope/pkg/validation"
4745
)
4846

@@ -181,7 +179,7 @@ func collectTestProfileBytes(t *testing.T) []byte {
181179

182180
func hugeProfileBytes(t *testing.T) []byte {
183181
t.Helper()
184-
b := testhelper2.NewProfileBuilderWithLabels(time.Now().UnixNano(), nil)
182+
b := pproftesthelper.NewProfileBuilderWithLabels(time.Now().UnixNano(), nil)
185183
p := b.CPUProfile()
186184
for i := 0; i < 10_000; i++ {
187185
p.ForStacktraceString(fmt.Sprintf("my_%d", i), "other").AddSamples(1)
@@ -1535,3 +1533,70 @@ func expectMetricsChange(t *testing.T, m1, m2, expectedChange map[prometheus.Col
15351533
assert.Equal(t, expectedDelta, delta, "metric %s", counter)
15361534
}
15371535
}
1536+
1537+
func TestPush_LabelRewrites(t *testing.T) {
1538+
ing := newFakeIngester(t, false)
1539+
d, err := New(Config{
1540+
DistributorRing: ringConfig,
1541+
}, testhelper.NewMockRing([]ring.InstanceDesc{
1542+
{Addr: "mock"},
1543+
{Addr: "mock"},
1544+
{Addr: "mock"},
1545+
}, 3), &poolFactory{f: func(addr string) (client.PoolClient, error) {
1546+
return ing, nil
1547+
}}, newOverrides(t), nil, log.NewLogfmtLogger(os.Stdout), nil)
1548+
require.NoError(t, err)
1549+
1550+
ctx := tenant.InjectTenantID(context.Background(), "user-1")
1551+
1552+
for idx, tc := range []struct {
1553+
name string
1554+
series []*typesv1.LabelPair
1555+
expectedSeries string
1556+
}{
1557+
{
1558+
name: "empty series",
1559+
series: []*typesv1.LabelPair{},
1560+
expectedSeries: `{__name__="process_cpu", service_name="unknown_service"}`,
1561+
},
1562+
{
1563+
name: "series with service_name labels",
1564+
series: []*typesv1.LabelPair{
1565+
{Name: "service_name", Value: "my-service"},
1566+
{Name: "cloud_region", Value: "my-region"},
1567+
},
1568+
expectedSeries: `{__name__="process_cpu", cloud_region="my-region", service_name="my-service"}`,
1569+
},
1570+
} {
1571+
t.Run(tc.name, func(t *testing.T) {
1572+
ing.mtx.Lock()
1573+
ing.requests = ing.requests[:0]
1574+
ing.mtx.Unlock()
1575+
1576+
p := pproftesthelper.NewProfileBuilderWithLabels(1000*int64(idx), tc.series).CPUProfile()
1577+
p.ForStacktraceString("world", "hello").AddSamples(1)
1578+
1579+
data, err := p.Profile.MarshalVT()
1580+
require.NoError(t, err)
1581+
1582+
_, err = d.Push(ctx, connect.NewRequest(&pushv1.PushRequest{
1583+
Series: []*pushv1.RawProfileSeries{
1584+
{
1585+
Labels: p.Labels,
1586+
Samples: []*pushv1.RawSample{
1587+
{RawProfile: data},
1588+
},
1589+
},
1590+
},
1591+
}))
1592+
require.NoError(t, err)
1593+
1594+
ing.mtx.Lock()
1595+
require.Len(t, ing.requests, 1)
1596+
require.Greater(t, len(ing.requests[0].Series), 1)
1597+
actualSeries := phlaremodel.LabelPairsString(ing.requests[0].Series[0].Labels)
1598+
assert.Equal(t, tc.expectedSeries, actualSeries)
1599+
ing.mtx.Unlock()
1600+
})
1601+
}
1602+
}

‎pkg/ingester/otlp/ingest_handler.go

Lines changed: 14 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@ import (
1919
pushv1 "github.com/grafana/pyroscope/api/gen/proto/go/push/v1"
2020
typesv1 "github.com/grafana/pyroscope/api/gen/proto/go/types/v1"
2121
distirbutormodel "github.com/grafana/pyroscope/pkg/distributor/model"
22+
"github.com/grafana/pyroscope/pkg/model"
2223
pyromodel "github.com/grafana/pyroscope/pkg/model"
2324
"github.com/grafana/pyroscope/pkg/pprof"
2425
"github.com/grafana/pyroscope/pkg/tenant"
@@ -150,18 +151,27 @@ func (h *ingestHandler) Export(ctx context.Context, er *pprofileotlp.ExportProfi
150151
}
151152

152153
// getServiceNameFromAttributes extracts service name from OTLP resource attributes.
153-
// Returns "unknown" if service name is not found or empty.
154+
// according to otel spec https://github.com/open-telemetry/opentelemetry-go/blob/ecfb73581f1b05af85fc393c3ce996a90cf2a5e2/semconv/v1.30.0/attribute_group.go#L10011-L10025
155+
// Returns "unknown_service:$process_name" if no service.name, but there is a process.executable.name
156+
// Returns "unknown_service" if no service.name and no process.executable.name
154157
func getServiceNameFromAttributes(attrs []*v1.KeyValue) string {
158+
fallback := model.AttrServiceNameFallback
155159
for _, attr := range attrs {
156-
if attr.Key == "service.name" {
160+
if attr.Key == string(model.AttrServiceName) {
157161
val := attr.GetValue()
158162
if sv := val.GetStringValue(); sv != "" {
159163
return sv
160164
}
161-
break
162165
}
166+
if attr.Key == string(model.AttrProcessExecutableName) {
167+
val := attr.GetValue()
168+
if sv := val.GetStringValue(); sv != "" {
169+
fallback += ":" + sv
170+
}
171+
}
172+
163173
}
164-
return "unknown"
174+
return fallback
165175
}
166176

167177
// getDefaultLabels returns the required base labels for Pyroscope profiles

‎pkg/ingester/otlp/ingest_handler_test.go

Lines changed: 35 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,21 @@ func TestGetServiceNameFromAttributes(t *testing.T) {
3131
{
3232
name: "empty attributes",
3333
attrs: []*v1.KeyValue{},
34-
expected: "unknown",
34+
expected: phlaremodel.AttrServiceNameFallback,
35+
},
36+
{
37+
name: "empty attributes",
38+
attrs: []*v1.KeyValue{
39+
{
40+
Key: "process.executable.name",
41+
Value: &v1.AnyValue{
42+
Value: &v1.AnyValue_StringValue{
43+
StringValue: "bash",
44+
},
45+
},
46+
},
47+
},
48+
expected: phlaremodel.AttrServiceNameFallback + ":bash",
3549
},
3650
{
3751
name: "service name present",
@@ -44,6 +58,14 @@ func TestGetServiceNameFromAttributes(t *testing.T) {
4458
},
4559
},
4660
},
61+
{
62+
Key: "process.executable.name",
63+
Value: &v1.AnyValue{
64+
Value: &v1.AnyValue_StringValue{
65+
StringValue: "test-executable",
66+
},
67+
},
68+
},
4769
},
4870
expected: "test-service",
4971
},
@@ -58,8 +80,16 @@ func TestGetServiceNameFromAttributes(t *testing.T) {
5880
},
5981
},
6082
},
83+
{
84+
Key: "process.executable.name",
85+
Value: &v1.AnyValue{
86+
Value: &v1.AnyValue_StringValue{
87+
StringValue: "",
88+
},
89+
},
90+
},
6191
},
62-
expected: "unknown",
92+
expected: phlaremodel.AttrServiceNameFallback,
6393
},
6494
{
6595
name: "service name among other attributes",
@@ -568,9 +598,9 @@ func TestDifferentServiceNames(t *testing.T) {
568598
require.Equal(t, 3, len(profiles[0].Series))
569599

570600
expectedProfiles := map[string]string{
571-
"{__delta__=\"false\", __name__=\"process_cpu\", __otel__=\"true\", service_name=\"service-a\"}": "testdata/TestDifferentServiceNames_service_a_profile.json",
572-
"{__delta__=\"false\", __name__=\"process_cpu\", __otel__=\"true\", service_name=\"service-b\"}": "testdata/TestDifferentServiceNames_service_b_profile.json",
573-
"{__delta__=\"false\", __name__=\"process_cpu\", __otel__=\"true\", service_name=\"unknown\"}": "testdata/TestDifferentServiceNames_unknown_profile.json",
601+
"{__delta__=\"false\", __name__=\"process_cpu\", __otel__=\"true\", service_name=\"service-a\"}": "testdata/TestDifferentServiceNames_service_a_profile.json",
602+
"{__delta__=\"false\", __name__=\"process_cpu\", __otel__=\"true\", service_name=\"service-b\"}": "testdata/TestDifferentServiceNames_service_b_profile.json",
603+
"{__delta__=\"false\", __name__=\"process_cpu\", __otel__=\"true\", service_name=\"unknown_service\"}": "testdata/TestDifferentServiceNames_unknown_profile.json",
574604
}
575605

576606
for _, s := range profiles[0].Series {

‎pkg/model/labels.go

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@ import (
1313
"github.com/cespare/xxhash/v2"
1414
pmodel "github.com/prometheus/common/model"
1515
"github.com/prometheus/prometheus/model/labels"
16+
semconv "go.opentelemetry.io/otel/semconv/v1.27.0"
1617

1718
typesv1 "github.com/grafana/pyroscope/api/gen/proto/go/types/v1"
1819
"github.com/grafana/pyroscope/pkg/util"
@@ -42,6 +43,11 @@ const (
4243

4344
LabelNamePyroscopeSpy = "pyroscope_spy"
4445

46+
AttrProcessExecutableName = semconv.ProcessExecutableNameKey
47+
48+
AttrServiceName = semconv.ServiceNameKey
49+
AttrServiceNameFallback = "unknown_service"
50+
4551
labelSep = '\xfe'
4652

4753
ProfileNameOffCpu = "off_cpu" // todo better name?

‎pkg/test/integration/ingest_otlp_test.go

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,7 @@ var otlpTestDatas = []otlpTestData{
3535
expectedProfiles: []expectedProfile{
3636
{
3737
"process_cpu:cpu:nanoseconds:cpu:nanoseconds",
38-
map[string]string{"service_name": "unknown"},
38+
map[string]string{"service_name": "unknown_service"},
3939
"testdata/otel-ebpf-profiler-unsymbolized.json",
4040
},
4141
},
@@ -49,12 +49,12 @@ var otlpTestDatas = []otlpTestData{
4949
expectedProfiles: []expectedProfile{
5050
{
5151
"process_cpu:cpu:nanoseconds:cpu:nanoseconds",
52-
map[string]string{"service_name": "unknown"},
52+
map[string]string{"service_name": "unknown_service"},
5353
"testdata/otel-ebpf-profiler-offcpu-cpu.json",
5454
},
5555
{
5656
"off_cpu:events:nanoseconds::",
57-
map[string]string{"service_name": "unknown"},
57+
map[string]string{"service_name": "unknown_service"},
5858
"testdata/otel-ebpf-profiler-offcpu.json",
5959
},
6060
},
@@ -68,7 +68,7 @@ var otlpTestDatas = []otlpTestData{
6868
expectedProfiles: []expectedProfile{
6969
{
7070
"process_cpu:cpu:nanoseconds:cpu:nanoseconds",
71-
map[string]string{"service_name": "unknown"},
71+
map[string]string{"service_name": "unknown_service"},
7272
"testdata/otel-ebpf-profiler-pyrosymbolized-unknown.json",
7373
},
7474
{

0 commit comments

Comments
 (0)