From 481c7c92ed146d8e16dc54e06e66b96d25e58486 Mon Sep 17 00:00:00 2001 From: Tolya Korniltsev Date: Wed, 18 Dec 2024 16:00:48 +0700 Subject: [PATCH 1/7] add pprof split test --- pkg/ingester/otlp/ingest_handler_test.go | 150 +++++++++++++++++++++++ 1 file changed, 150 insertions(+) diff --git a/pkg/ingester/otlp/ingest_handler_test.go b/pkg/ingester/otlp/ingest_handler_test.go index 4d26f6c98a..b3fd0a03aa 100644 --- a/pkg/ingester/otlp/ingest_handler_test.go +++ b/pkg/ingester/otlp/ingest_handler_test.go @@ -314,6 +314,156 @@ func TestSampleAttributes(t *testing.T) { assert.Equal(t, "(process = firefox) ||| firefox.so 0x2e;firefox.so 0x1e 239", ss[1]) } +func TestDifferentServiceNames(t *testing.T) { + // Create a profile with two samples having different service.name attributes + // Expect them to be pushed as separate profiles + svc := mockotlp.NewMockPushService(t) + var profiles []*pushv1.PushRequest + svc.On("Push", mock.Anything, mock.Anything).Run(func(args mock.Arguments) { + c := (args.Get(1)).(*connect.Request[pushv1.PushRequest]) + profiles = append(profiles, c.Msg) + }).Return(nil, nil) + + otlpb := new(otlpbuilder) + // Create mappings for both services + otlpb.profile.Mapping = []*v1experimental.Mapping{{ + MemoryStart: 0x1000, + MemoryLimit: 0x2000, + Filename: otlpb.addstr("service-a.so"), + }, { + MemoryStart: 0x2000, + MemoryLimit: 0x3000, + Filename: otlpb.addstr("service-b.so"), + }} + + // Create different locations for each service + otlpb.profile.Location = []*v1experimental.Location{{ + MappingIndex: 0, // service-a.so + Address: 0x1100, + Line: []*v1experimental.Line{{ + FunctionIndex: 0, + Line: 10, + }}, + }, { + MappingIndex: 0, // service-a.so + Address: 0x1200, + Line: []*v1experimental.Line{{ + FunctionIndex: 1, + Line: 20, + }}, + }, { + MappingIndex: 1, // service-b.so + Address: 0x2100, + Line: []*v1experimental.Line{{ + FunctionIndex: 2, + Line: 30, + }}, + }, { + MappingIndex: 1, // service-b.so + Address: 0x2200, + Line: []*v1experimental.Line{{ + FunctionIndex: 3, + Line: 40, + }}, + }} + + // Add functions + otlpb.profile.Function = []*v1experimental.Function{{ + Name: otlpb.addstr("serviceA_func1"), + SystemName: otlpb.addstr("serviceA_func1"), + Filename: otlpb.addstr("service_a.go"), + }, { + Name: otlpb.addstr("serviceA_func2"), + SystemName: otlpb.addstr("serviceA_func2"), + Filename: otlpb.addstr("service_a.go"), + }, { + Name: otlpb.addstr("serviceB_func1"), + SystemName: otlpb.addstr("serviceB_func1"), + Filename: otlpb.addstr("service_b.go"), + }, { + Name: otlpb.addstr("serviceB_func2"), + SystemName: otlpb.addstr("serviceB_func2"), + Filename: otlpb.addstr("service_b.go"), + }} + + otlpb.profile.LocationIndices = []int64{0, 1, 2, 3} + + // Create two samples with different service.name attributes and different stacktraces + otlpb.profile.Sample = []*v1experimental.Sample{{ + LocationsStartIndex: 0, + LocationsLength: 2, // Use first two locations + Value: []int64{100}, + Attributes: []uint64{0}, + }, { + LocationsStartIndex: 2, + LocationsLength: 2, // Use last two locations + Value: []int64{200}, + Attributes: []uint64{1}, + }} + + // Set up the attribute table with different service names + otlpb.profile.AttributeTable = []v1.KeyValue{{ + Key: "service.name", + Value: v1.AnyValue{ + Value: &v1.AnyValue_StringValue{ + StringValue: "service-a", + }, + }, + }, { + Key: "service.name", + Value: v1.AnyValue{ + Value: &v1.AnyValue_StringValue{ + StringValue: "service-b", + }, + }, + }} + + req := &v1experimental2.ExportProfilesServiceRequest{ + ResourceProfiles: []*v1experimental.ResourceProfiles{{ + ScopeProfiles: []*v1experimental.ScopeProfiles{{ + Profiles: []*v1experimental.ProfileContainer{{ + Profile: &otlpb.profile, + }}}}}}} + + logger := testutil.NewLogger(t) + h := NewOTLPIngestHandler(svc, logger, false) + _, err := h.Export(context.Background(), req) + require.NoError(t, err) + + // We should have two separate profiles + require.Equal(t, 2, len(profiles)) + + expectedStacks := map[string]string{ + "service-a": " ||| serviceA_func2;serviceA_func1 100", + "service-b": " ||| serviceB_func2;serviceB_func1 200", + } + + // Verify service names and stacktraces in the profiles + for _, p := range profiles { + require.Equal(t, 1, len(p.Series)) + seriesLabelsMap := make(map[string]string) + for _, label := range p.Series[0].Labels { + seriesLabelsMap[label.Name] = label.Value + } + + serviceName := seriesLabelsMap["service_name"] + require.Contains(t, []string{"service-a", "service-b"}, serviceName) + + // Verify the stacktrace + gp := new(googlev1.Profile) + err = gp.UnmarshalVT(p.Series[0].Samples[0].RawProfile) + require.NoError(t, err) + + ss := bench.StackCollapseProtoWithOptions(gp, bench.StackCollapseOptions{ + ValueIdx: 0, + Scale: 1, + WithLabels: true, + }) + require.Equal(t, 1, len(ss)) + assert.Equal(t, expectedStacks[serviceName], ss[0]) + } +} + type otlpbuilder struct { profile v1experimental.Profile stringmap map[string]int64 From 517046afefc763688a4beabcc126fdd82c8c7383 Mon Sep 17 00:00:00 2001 From: Tolya Korniltsev Date: Wed, 18 Dec 2024 16:56:00 +0700 Subject: [PATCH 2/7] update conversion --- pkg/ingester/otlp/convert.go | 256 +++++++++++++++++++++-------------- 1 file changed, 151 insertions(+), 105 deletions(-) diff --git a/pkg/ingester/otlp/convert.go b/pkg/ingester/otlp/convert.go index 12b86bbaf5..80fc08eaf1 100644 --- a/pkg/ingester/otlp/convert.go +++ b/pkg/ingester/otlp/convert.go @@ -15,89 +15,109 @@ func OprofToPprof(p *otelProfile.Profile) ([]byte, error) { return proto.Marshal(dst) } -// ConvertOtelToGoogle converts an OpenTelemetry profile to a Google profile. -func ConvertOtelToGoogle(src *otelProfile.Profile) *googleProfile.Profile { - dst := &googleProfile.Profile{ - SampleType: convertSampleTypesBack(src.SampleType), - StringTable: src.StringTable[:], - TimeNanos: src.TimeNanos, - DurationNanos: src.DurationNanos, - PeriodType: convertValueTypeBack(src.PeriodType), - Period: src.Period, - DefaultSampleType: src.DefaultSampleType, - DropFrames: src.DropFrames, - KeepFrames: src.KeepFrames, - Comment: src.Comment, - Mapping: convertMappingsBack(src.Mapping), - } - stringmap := make(map[string]int) - addstr := func(s string) int64 { - if _, ok := stringmap[s]; !ok { - stringmap[s] = len(dst.StringTable) - dst.StringTable = append(dst.StringTable, s) - } - return int64(stringmap[s]) - } - addstr("") +type profileBuilder struct { + src *otelProfile.Profile + dst *googleProfile.Profile + stringMap map[string]int64 + functionMap map[*otelProfile.Function]uint64 + unsymbolziedFuncNameMap map[string]uint64 + locationMap map[*otelProfile.Location]uint64 + mappingMap map[*otelProfile.Mapping]uint64 +} - if dst.TimeNanos == 0 { - dst.TimeNanos = time.Now().UnixNano() +func newProfileBuilder(src *otelProfile.Profile) *profileBuilder { + res := &profileBuilder{ + src: src, + stringMap: make(map[string]int64), + functionMap: make(map[*otelProfile.Function]uint64), + locationMap: make(map[*otelProfile.Location]uint64), + mappingMap: make(map[*otelProfile.Mapping]uint64), + unsymbolziedFuncNameMap: make(map[string]uint64), + dst: &googleProfile.Profile{ + SampleType: convertSampleTypesBack(src.SampleType), + TimeNanos: src.TimeNanos, + DurationNanos: src.DurationNanos, + PeriodType: convertValueTypeBack(src.PeriodType), + Period: src.Period, + DefaultSampleType: src.DefaultSampleType, + DropFrames: src.DropFrames, + KeepFrames: src.KeepFrames, + Comment: src.Comment, + }, + } + res.addstr("") + if len(res.dst.SampleType) == 0 { + res.dst.SampleType = []*googleProfile.ValueType{{ + Type: res.addstr("samples"), + Unit: res.addstr("ms"), + }} + res.dst.DefaultSampleType = res.addstr("samples") } - if dst.DurationNanos == 0 { - dst.DurationNanos = (time.Second * 10).Nanoseconds() + if res.dst.TimeNanos == 0 { + res.dst.TimeNanos = time.Now().UnixNano() } + if res.dst.DurationNanos == 0 { + res.dst.DurationNanos = (time.Second * 10).Nanoseconds() + } + return res +} - // attribute_table - // attribute_units - // link_table +func (p *profileBuilder) addstr(s string) int64 { + if i, ok := p.stringMap[s]; ok { + return i + } + idx := int64(len(p.dst.StringTable)) + p.stringMap[s] = idx + p.dst.StringTable = append(p.dst.StringTable, s) + return idx +} - dst.Function = []*googleProfile.Function{} - for i, funcItem := range src.Function { - gf := convertFunctionBack(funcItem) - gf.Id = uint64(i + 1) - dst.Function = append(dst.Function, gf) +func (p *profileBuilder) addfunc(s string) uint64 { + if i, ok := p.unsymbolziedFuncNameMap[s]; ok { + return i } - funcmap := map[string]uint64{} - addfunc := func(s string) uint64 { - if _, ok := funcmap[s]; !ok { - funcmap[s] = uint64(len(dst.Function)) + 1 - dst.Function = append(dst.Function, &googleProfile.Function{ - Id: funcmap[s], - Name: addstr(s), - }) - } - return funcmap[s] - } - - dst.Location = []*googleProfile.Location{} - // Convert locations and mappings - for i, loc := range src.Location { - gl := convertLocationBack(loc) - gl.Id = uint64(i + 1) - if len(gl.Line) == 0 { - m := src.Mapping[loc.MappingIndex] - gl.Line = append(gl.Line, &googleProfile.Line{ - FunctionId: addfunc(fmt.Sprintf("%s 0x%x", src.StringTable[m.Filename], loc.Address)), - }) - } - dst.Location = append(dst.Location, gl) + idx := uint64(len(p.dst.Function)) + 1 + p.unsymbolziedFuncNameMap[s] = idx + gf := &googleProfile.Function{ + Id: idx, + Name: p.addstr(s), } + p.dst.Function = append(p.dst.Function, gf) + return idx +} + +func (p *profileBuilder) build() *googleProfile.Profile { + return p.dst +} - // Convert samples +// ConvertOtelToGoogle converts an OpenTelemetry profile to a Google profile. +func ConvertOtelToGoogle(src *otelProfile.Profile) *googleProfile.Profile { + svc2Profile := make(map[string]*profileBuilder) for _, sample := range src.Sample { - gs := convertSampleBack(src, sample, src.LocationIndices, addstr) - dst.Sample = append(dst.Sample, gs) + svc := serviceNameFromSample(src, sample) + p, ok := svc2Profile[svc] + if !ok { + p = newProfileBuilder(src) + svc2Profile[svc] = p + } + p.convertSampleBack(sample) } - if len(dst.SampleType) == 0 { - dst.SampleType = []*googleProfile.ValueType{{ - Type: addstr("samples"), - Unit: addstr("ms"), - }} - dst.DefaultSampleType = addstr("samples") + for _, p := range svc2Profile { + return p.build() } - return dst + return nil +} + +func serviceNameFromSample(p *otelProfile.Profile, sample *otelProfile.Sample) string { + //for _, attributeIndex := range sample.Attributes { + // attribute := p.AttributeTable[attributeIndex] + // if attribute.Key == "service.name" { + // return attribute.Value.GetStringValue() + // } + //} + return "" } func convertSampleTypesBack(ost []*otelProfile.ValueType) []*googleProfile.ValueType { @@ -121,37 +141,58 @@ func convertValueTypeBack(ovt *otelProfile.ValueType) *googleProfile.ValueType { } } -func convertLocationBack(ol *otelProfile.Location) *googleProfile.Location { +func (p *profileBuilder) convertLocationBack(ol *otelProfile.Location) uint64 { + if i, ok := p.locationMap[ol]; ok { + return i + } + om := p.src.Mapping[ol.MappingIndex] gl := &googleProfile.Location{ - MappingId: ol.MappingIndex + 1, + MappingId: p.convertMappingBack(om), Address: ol.Address, Line: make([]*googleProfile.Line, len(ol.Line)), IsFolded: ol.IsFolded, } for i, line := range ol.Line { - gl.Line[i] = convertLineBack(line) + gl.Line[i] = p.convertLineBack(line) } - return gl + + if len(gl.Line) == 0 { + gl.Line = append(gl.Line, &googleProfile.Line{ + FunctionId: p.addfunc(fmt.Sprintf("%s 0x%x", p.src.StringTable[om.Filename], ol.Address)), + }) + } + + p.dst.Location = append(p.dst.Location, gl) + gl.Id = uint64(len(p.dst.Location)) + p.locationMap[ol] = gl.Id + return gl.Id } // convertLineBack converts an OpenTelemetry Line to a Google Line. -func convertLineBack(ol *otelProfile.Line) *googleProfile.Line { +func (p *profileBuilder) convertLineBack(ol *otelProfile.Line) *googleProfile.Line { return &googleProfile.Line{ - FunctionId: ol.FunctionIndex + 1, + FunctionId: p.convertFunctionBack(p.src.Function[ol.FunctionIndex]), Line: ol.Line, } } -func convertFunctionBack(of *otelProfile.Function) *googleProfile.Function { - return &googleProfile.Function{ - Name: of.Name, - SystemName: of.SystemName, - Filename: of.Filename, +func (p *profileBuilder) convertFunctionBack(of *otelProfile.Function) uint64 { + if i, ok := p.functionMap[of]; ok { + return i + } + gf := &googleProfile.Function{ + Name: p.addstr(p.src.StringTable[of.Name]), + SystemName: p.addstr(p.src.StringTable[of.SystemName]), + Filename: p.addstr(p.src.StringTable[of.Filename]), StartLine: of.StartLine, } + p.dst.Function = append(p.dst.Function, gf) + gf.Id = uint64(len(p.dst.Function)) + p.functionMap[of] = gf.Id + return gf.Id } -func convertSampleBack(p *otelProfile.Profile, os *otelProfile.Sample, locationIndexes []int64, addstr func(s string) int64) *googleProfile.Sample { +func (p *profileBuilder) convertSampleBack(os *otelProfile.Sample) *googleProfile.Sample { gs := &googleProfile.Sample{ Value: os.Value, } @@ -159,44 +200,49 @@ func convertSampleBack(p *otelProfile.Profile, os *otelProfile.Sample, locationI if len(gs.Value) == 0 { gs.Value = []int64{int64(len(os.TimestampsUnixNano))} } - convertSampleAttributesToLabelsBack(p, os, gs, addstr) + p.convertSampleAttributesToLabelsBack(os, gs) for i := os.LocationsStartIndex; i < os.LocationsStartIndex+os.LocationsLength; i++ { - gs.LocationId = append(gs.LocationId, uint64(locationIndexes[i]+1)) + gs.LocationId = append(gs.LocationId, p.convertLocationBack(p.src.Location[p.src.LocationIndices[i]])) } + p.dst.Sample = append(p.dst.Sample, gs) + return gs } -func convertSampleAttributesToLabelsBack(p *otelProfile.Profile, os *otelProfile.Sample, gs *googleProfile.Sample, addstr func(s string) int64) { +func (p *profileBuilder) convertSampleAttributesToLabelsBack(os *otelProfile.Sample, gs *googleProfile.Sample) { gs.Label = make([]*googleProfile.Label, 0, len(os.Attributes)) for _, attribute := range os.Attributes { - att := p.AttributeTable[attribute] + att := p.src.AttributeTable[attribute] if att.Value.GetStringValue() != "" { gs.Label = append(gs.Label, &googleProfile.Label{ - Key: addstr(att.Key), - Str: addstr(att.Value.GetStringValue()), + Key: p.addstr(att.Key), + Str: p.addstr(att.Value.GetStringValue()), }) } } } // convertMappingsBack converts a slice of OpenTelemetry Mapping entries to Google Mapping entries. -func convertMappingsBack(otelMappings []*otelProfile.Mapping) []*googleProfile.Mapping { - googleMappings := make([]*googleProfile.Mapping, len(otelMappings)) - for i, om := range otelMappings { - googleMappings[i] = &googleProfile.Mapping{ - Id: uint64(i + 1), // Assuming direct mapping of IDs - MemoryStart: om.MemoryStart, - MemoryLimit: om.MemoryLimit, - FileOffset: om.FileOffset, - Filename: om.Filename, // Assume direct use; may need conversion if using indices - BuildId: om.BuildId, - HasFunctions: om.HasFunctions, - HasFilenames: om.HasFilenames, - HasLineNumbers: om.HasLineNumbers, - HasInlineFrames: om.HasInlineFrames, - } - } - return googleMappings +func (p *profileBuilder) convertMappingBack(om *otelProfile.Mapping) uint64 { + if i, ok := p.mappingMap[om]; ok { + return i + } + + gm := &googleProfile.Mapping{ + MemoryStart: om.MemoryStart, + MemoryLimit: om.MemoryLimit, + FileOffset: om.FileOffset, + Filename: p.addstr(p.src.StringTable[om.Filename]), + BuildId: p.addstr(p.src.StringTable[om.BuildId]), + HasFunctions: om.HasFunctions, + HasFilenames: om.HasFilenames, + HasLineNumbers: om.HasLineNumbers, + HasInlineFrames: om.HasInlineFrames, + } + p.dst.Mapping = append(p.dst.Mapping, gm) + gm.Id = uint64(len(p.dst.Mapping)) + p.mappingMap[om] = gm.Id + return gm.Id } From 6d23ce5f0758e4c28b63e0e4d086860331df52d6 Mon Sep 17 00:00:00 2001 From: Tolya Korniltsev Date: Wed, 18 Dec 2024 17:07:54 +0700 Subject: [PATCH 3/7] fix test --- pkg/ingester/otlp/convert.go | 32 ++++------ pkg/ingester/otlp/ingest_handler.go | 79 ++++++++++++------------ pkg/ingester/otlp/ingest_handler_test.go | 38 ++++++------ 3 files changed, 69 insertions(+), 80 deletions(-) diff --git a/pkg/ingester/otlp/convert.go b/pkg/ingester/otlp/convert.go index 80fc08eaf1..191c118c56 100644 --- a/pkg/ingester/otlp/convert.go +++ b/pkg/ingester/otlp/convert.go @@ -6,15 +6,8 @@ import ( googleProfile "github.com/grafana/pyroscope/api/gen/proto/go/google/v1" otelProfile "github.com/grafana/pyroscope/api/otlp/profiles/v1experimental" - - "google.golang.org/protobuf/proto" ) -func OprofToPprof(p *otelProfile.Profile) ([]byte, error) { - dst := ConvertOtelToGoogle(p) - return proto.Marshal(dst) -} - type profileBuilder struct { src *otelProfile.Profile dst *googleProfile.Profile @@ -86,12 +79,8 @@ func (p *profileBuilder) addfunc(s string) uint64 { return idx } -func (p *profileBuilder) build() *googleProfile.Profile { - return p.dst -} - // ConvertOtelToGoogle converts an OpenTelemetry profile to a Google profile. -func ConvertOtelToGoogle(src *otelProfile.Profile) *googleProfile.Profile { +func ConvertOtelToGoogle(src *otelProfile.Profile) map[string]*googleProfile.Profile { svc2Profile := make(map[string]*profileBuilder) for _, sample := range src.Sample { svc := serviceNameFromSample(src, sample) @@ -103,20 +92,21 @@ func ConvertOtelToGoogle(src *otelProfile.Profile) *googleProfile.Profile { p.convertSampleBack(sample) } - for _, p := range svc2Profile { - return p.build() + result := make(map[string]*googleProfile.Profile) + for svc, p := range svc2Profile { + result[svc] = p.dst } - return nil + return result } func serviceNameFromSample(p *otelProfile.Profile, sample *otelProfile.Sample) string { - //for _, attributeIndex := range sample.Attributes { - // attribute := p.AttributeTable[attributeIndex] - // if attribute.Key == "service.name" { - // return attribute.Value.GetStringValue() - // } - //} + for _, attributeIndex := range sample.Attributes { + attribute := p.AttributeTable[attributeIndex] + if attribute.Key == "service.name" { + return attribute.Value.GetStringValue() + } + } return "" } diff --git a/pkg/ingester/otlp/ingest_handler.go b/pkg/ingester/otlp/ingest_handler.go index a88d70d94f..71ab6b1826 100644 --- a/pkg/ingester/otlp/ingest_handler.go +++ b/pkg/ingester/otlp/ingest_handler.go @@ -84,51 +84,54 @@ func (h *ingestHandler) Export(ctx context.Context, er *pprofileotlp.ExportProfi for i := 0; i < len(rps); i++ { rp := rps[i] - // Get service name serviceName := getServiceNameFromAttributes(rp.Resource.GetAttributes()) - // Start with default labels - labels := getDefaultLabels(serviceName) - - // Track processed attribute keys to avoid duplicates across levels - processedKeys := make(map[string]bool) - - // Add resource attributes - labels = appendAttributesUnique(labels, rp.Resource.GetAttributes(), processedKeys) - sps := rp.ScopeProfiles for j := 0; j < len(sps); j++ { sp := sps[j] - // Add scope attributes - labels = appendAttributesUnique(labels, sp.Scope.GetAttributes(), processedKeys) - for k := 0; k < len(sp.Profiles); k++ { p := sp.Profiles[k] - // Add profile attributes - labels = appendAttributesUnique(labels, p.GetAttributes(), processedKeys) - - pprofBytes, err := OprofToPprof(p.Profile) - if err != nil { - return &pprofileotlp.ExportProfilesServiceResponse{}, fmt.Errorf("failed to convert from OTLP to legacy pprof: %w", err) - } - - req := &pushv1.PushRequest{ - Series: []*pushv1.RawProfileSeries{ - { - Labels: labels, - Samples: []*pushv1.RawSample{{ - RawProfile: pprofBytes, - ID: uuid.New().String(), - }}, + pprofProfiles := ConvertOtelToGoogle(p.Profile) + + for samplesServiceName, pprofProfile := range pprofProfiles { + labels := getDefaultLabels() + processedKeys := make(map[string]bool) + labels = appendAttributesUnique(labels, rp.Resource.GetAttributes(), processedKeys) + labels = appendAttributesUnique(labels, sp.Scope.GetAttributes(), processedKeys) + labels = appendAttributesUnique(labels, p.GetAttributes(), processedKeys) + svc := samplesServiceName + if svc == "" { + svc = serviceName + } + labels = append(labels, &typesv1.LabelPair{ + Name: "service_name", + Value: svc, + }) + + pprofBytes, err := pprofProfile.MarshalVT() + if err != nil { + return &pprofileotlp.ExportProfilesServiceResponse{}, fmt.Errorf("failed to marshal pprof profile: %w", err) + } + + req := &pushv1.PushRequest{ + Series: []*pushv1.RawProfileSeries{ + { + Labels: labels, + Samples: []*pushv1.RawSample{{ + RawProfile: pprofBytes, + ID: uuid.New().String(), + }}, + }, }, - }, - } - _, err = h.svc.Push(ctx, connect.NewRequest(req)) - if err != nil { - h.log.Log("msg", "failed to push profile", "err", err) - return &pprofileotlp.ExportProfilesServiceResponse{}, fmt.Errorf("failed to make a GRPC request: %w", err) + } + + _, err = h.svc.Push(ctx, connect.NewRequest(req)) + if err != nil { + h.log.Log("msg", "failed to push profile", "err", err) + return &pprofileotlp.ExportProfilesServiceResponse{}, fmt.Errorf("failed to make a GRPC request: %w", err) + } } } } @@ -153,16 +156,12 @@ func getServiceNameFromAttributes(attrs []v1.KeyValue) string { } // getDefaultLabels returns the required base labels for Pyroscope profiles -func getDefaultLabels(serviceName string) []*typesv1.LabelPair { +func getDefaultLabels() []*typesv1.LabelPair { return []*typesv1.LabelPair{ { Name: "__name__", Value: "process_cpu", }, - { - Name: "service_name", - Value: serviceName, - }, { Name: "__delta__", Value: "false", diff --git a/pkg/ingester/otlp/ingest_handler_test.go b/pkg/ingester/otlp/ingest_handler_test.go index b3fd0a03aa..e5d19fb3ca 100644 --- a/pkg/ingester/otlp/ingest_handler_test.go +++ b/pkg/ingester/otlp/ingest_handler_test.go @@ -342,48 +342,48 @@ func TestDifferentServiceNames(t *testing.T) { Address: 0x1100, Line: []*v1experimental.Line{{ FunctionIndex: 0, - Line: 10, + Line: 10, }}, }, { MappingIndex: 0, // service-a.so Address: 0x1200, Line: []*v1experimental.Line{{ FunctionIndex: 1, - Line: 20, + Line: 20, }}, }, { MappingIndex: 1, // service-b.so Address: 0x2100, Line: []*v1experimental.Line{{ FunctionIndex: 2, - Line: 30, + Line: 30, }}, }, { MappingIndex: 1, // service-b.so Address: 0x2200, Line: []*v1experimental.Line{{ FunctionIndex: 3, - Line: 40, + Line: 40, }}, }} // Add functions otlpb.profile.Function = []*v1experimental.Function{{ - Name: otlpb.addstr("serviceA_func1"), + Name: otlpb.addstr("serviceA_func1"), SystemName: otlpb.addstr("serviceA_func1"), - Filename: otlpb.addstr("service_a.go"), + Filename: otlpb.addstr("service_a.go"), }, { - Name: otlpb.addstr("serviceA_func2"), + Name: otlpb.addstr("serviceA_func2"), SystemName: otlpb.addstr("serviceA_func2"), - Filename: otlpb.addstr("service_a.go"), + Filename: otlpb.addstr("service_a.go"), }, { - Name: otlpb.addstr("serviceB_func1"), + Name: otlpb.addstr("serviceB_func1"), SystemName: otlpb.addstr("serviceB_func1"), - Filename: otlpb.addstr("service_b.go"), + Filename: otlpb.addstr("service_b.go"), }, { - Name: otlpb.addstr("serviceB_func2"), + Name: otlpb.addstr("serviceB_func2"), SystemName: otlpb.addstr("serviceB_func2"), - Filename: otlpb.addstr("service_b.go"), + Filename: otlpb.addstr("service_b.go"), }} otlpb.profile.LocationIndices = []int64{0, 1, 2, 3} @@ -392,13 +392,13 @@ func TestDifferentServiceNames(t *testing.T) { otlpb.profile.Sample = []*v1experimental.Sample{{ LocationsStartIndex: 0, LocationsLength: 2, // Use first two locations - Value: []int64{100}, - Attributes: []uint64{0}, + Value: []int64{100}, + Attributes: []uint64{0}, }, { LocationsStartIndex: 2, LocationsLength: 2, // Use last two locations - Value: []int64{200}, - Attributes: []uint64{1}, + Value: []int64{200}, + Attributes: []uint64{1}, }} // Set up the attribute table with different service names @@ -434,8 +434,8 @@ func TestDifferentServiceNames(t *testing.T) { require.Equal(t, 2, len(profiles)) expectedStacks := map[string]string{ - "service-a": " ||| serviceA_func2;serviceA_func1 100", - "service-b": " ||| serviceB_func2;serviceB_func1 200", + "service-a": "(service.name = service-a) ||| serviceA_func2;serviceA_func1 100", + "service-b": "(service.name = service-b) ||| serviceB_func2;serviceB_func1 200", } // Verify service names and stacktraces in the profiles @@ -445,7 +445,7 @@ func TestDifferentServiceNames(t *testing.T) { for _, label := range p.Series[0].Labels { seriesLabelsMap[label.Name] = label.Value } - + serviceName := seriesLabelsMap["service_name"] require.Contains(t, []string{"service-a", "service-b"}, serviceName) From 921324cbfb87b0501c57074c1ca34b738460c667 Mon Sep 17 00:00:00 2001 From: Tolya Korniltsev Date: Wed, 18 Dec 2024 17:17:18 +0700 Subject: [PATCH 4/7] fix sample type and value types --- pkg/ingester/otlp/convert.go | 58 ++++++++++++------------ pkg/ingester/otlp/ingest_handler_test.go | 27 ++++++++++- 2 files changed, 54 insertions(+), 31 deletions(-) diff --git a/pkg/ingester/otlp/convert.go b/pkg/ingester/otlp/convert.go index 191c118c56..02c2498a45 100644 --- a/pkg/ingester/otlp/convert.go +++ b/pkg/ingester/otlp/convert.go @@ -8,6 +8,27 @@ import ( otelProfile "github.com/grafana/pyroscope/api/otlp/profiles/v1experimental" ) +// ConvertOtelToGoogle converts an OpenTelemetry profile to a Google profile. +func ConvertOtelToGoogle(src *otelProfile.Profile) map[string]*googleProfile.Profile { + svc2Profile := make(map[string]*profileBuilder) + for _, sample := range src.Sample { + svc := serviceNameFromSample(src, sample) + p, ok := svc2Profile[svc] + if !ok { + p = newProfileBuilder(src) + svc2Profile[svc] = p + } + p.convertSampleBack(sample) + } + + result := make(map[string]*googleProfile.Profile) + for svc, p := range svc2Profile { + result[svc] = p.dst + } + + return result +} + type profileBuilder struct { src *otelProfile.Profile dst *googleProfile.Profile @@ -27,10 +48,8 @@ func newProfileBuilder(src *otelProfile.Profile) *profileBuilder { mappingMap: make(map[*otelProfile.Mapping]uint64), unsymbolziedFuncNameMap: make(map[string]uint64), dst: &googleProfile.Profile{ - SampleType: convertSampleTypesBack(src.SampleType), TimeNanos: src.TimeNanos, DurationNanos: src.DurationNanos, - PeriodType: convertValueTypeBack(src.PeriodType), Period: src.Period, DefaultSampleType: src.DefaultSampleType, DropFrames: src.DropFrames, @@ -39,6 +58,8 @@ func newProfileBuilder(src *otelProfile.Profile) *profileBuilder { }, } res.addstr("") + res.dst.SampleType = res.convertSampleTypesBack(src.SampleType) + res.dst.PeriodType = res.convertValueTypeBack(src.PeriodType) if len(res.dst.SampleType) == 0 { res.dst.SampleType = []*googleProfile.ValueType{{ Type: res.addstr("samples"), @@ -79,27 +100,6 @@ func (p *profileBuilder) addfunc(s string) uint64 { return idx } -// ConvertOtelToGoogle converts an OpenTelemetry profile to a Google profile. -func ConvertOtelToGoogle(src *otelProfile.Profile) map[string]*googleProfile.Profile { - svc2Profile := make(map[string]*profileBuilder) - for _, sample := range src.Sample { - svc := serviceNameFromSample(src, sample) - p, ok := svc2Profile[svc] - if !ok { - p = newProfileBuilder(src) - svc2Profile[svc] = p - } - p.convertSampleBack(sample) - } - - result := make(map[string]*googleProfile.Profile) - for svc, p := range svc2Profile { - result[svc] = p.dst - } - - return result -} - func serviceNameFromSample(p *otelProfile.Profile, sample *otelProfile.Sample) string { for _, attributeIndex := range sample.Attributes { attribute := p.AttributeTable[attributeIndex] @@ -110,24 +110,24 @@ func serviceNameFromSample(p *otelProfile.Profile, sample *otelProfile.Sample) s return "" } -func convertSampleTypesBack(ost []*otelProfile.ValueType) []*googleProfile.ValueType { +func (p *profileBuilder) convertSampleTypesBack(ost []*otelProfile.ValueType) []*googleProfile.ValueType { var gst []*googleProfile.ValueType for _, st := range ost { gst = append(gst, &googleProfile.ValueType{ - Type: st.Type, - Unit: st.Unit, + Type: p.addstr(p.src.StringTable[st.Type]), + Unit: p.addstr(p.src.StringTable[st.Unit]), }) } return gst } -func convertValueTypeBack(ovt *otelProfile.ValueType) *googleProfile.ValueType { +func (p *profileBuilder) convertValueTypeBack(ovt *otelProfile.ValueType) *googleProfile.ValueType { if ovt == nil { return nil } return &googleProfile.ValueType{ - Type: ovt.Type, - Unit: ovt.Unit, + Type: p.addstr(p.src.StringTable[ovt.Type]), + Unit: p.addstr(p.src.StringTable[ovt.Unit]), } } diff --git a/pkg/ingester/otlp/ingest_handler_test.go b/pkg/ingester/otlp/ingest_handler_test.go index e5d19fb3ca..b8b32c7eb6 100644 --- a/pkg/ingester/otlp/ingest_handler_test.go +++ b/pkg/ingester/otlp/ingest_handler_test.go @@ -418,6 +418,17 @@ func TestDifferentServiceNames(t *testing.T) { }, }} + // Add sample types and period type + otlpb.profile.SampleType = []*v1experimental.ValueType{{ + Type: otlpb.addstr("cpu"), + Unit: otlpb.addstr("nanoseconds"), + }} + otlpb.profile.PeriodType = &v1experimental.ValueType{ + Type: otlpb.addstr("cpu"), + Unit: otlpb.addstr("nanoseconds"), + } + otlpb.profile.Period = 10000000 // 10ms + req := &v1experimental2.ExportProfilesServiceRequest{ ResourceProfiles: []*v1experimental.ResourceProfiles{{ ScopeProfiles: []*v1experimental.ScopeProfiles{{ @@ -438,7 +449,7 @@ func TestDifferentServiceNames(t *testing.T) { "service-b": "(service.name = service-b) ||| serviceB_func2;serviceB_func1 200", } - // Verify service names and stacktraces in the profiles + // Verify service names, stacktraces, and profile metadata in the profiles for _, p := range profiles { require.Equal(t, 1, len(p.Series)) seriesLabelsMap := make(map[string]string) @@ -449,11 +460,23 @@ func TestDifferentServiceNames(t *testing.T) { serviceName := seriesLabelsMap["service_name"] require.Contains(t, []string{"service-a", "service-b"}, serviceName) - // Verify the stacktrace + // Verify the profile contents gp := new(googlev1.Profile) err = gp.UnmarshalVT(p.Series[0].Samples[0].RawProfile) require.NoError(t, err) + // Verify sample types + require.Equal(t, 1, len(gp.SampleType)) + assert.Equal(t, "cpu", gp.StringTable[gp.SampleType[0].Type]) + assert.Equal(t, "nanoseconds", gp.StringTable[gp.SampleType[0].Unit]) + + // Verify period type + require.NotNil(t, gp.PeriodType) + assert.Equal(t, "cpu", gp.StringTable[gp.PeriodType.Type]) + assert.Equal(t, "nanoseconds", gp.StringTable[gp.PeriodType.Unit]) + assert.Equal(t, int64(10000000), gp.Period) + + // Verify stacktraces ss := bench.StackCollapseProtoWithOptions(gp, bench.StackCollapseOptions{ ValueIdx: 0, Scale: 1, From e345ace7c95218921a85b07feae9d4b7d76ae925 Mon Sep 17 00:00:00 2001 From: Tolya Korniltsev Date: Thu, 19 Dec 2024 12:57:39 +0700 Subject: [PATCH 5/7] do not include service.name label in the samples labels --- pkg/ingester/otlp/convert.go | 3 +++ pkg/ingester/otlp/ingest_handler_test.go | 7 +++++-- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/pkg/ingester/otlp/convert.go b/pkg/ingester/otlp/convert.go index 02c2498a45..10842e96f1 100644 --- a/pkg/ingester/otlp/convert.go +++ b/pkg/ingester/otlp/convert.go @@ -205,6 +205,9 @@ func (p *profileBuilder) convertSampleAttributesToLabelsBack(os *otelProfile.Sam gs.Label = make([]*googleProfile.Label, 0, len(os.Attributes)) for _, attribute := range os.Attributes { att := p.src.AttributeTable[attribute] + if att.Key == "service.name" { + continue + } if att.Value.GetStringValue() != "" { gs.Label = append(gs.Label, &googleProfile.Label{ Key: p.addstr(att.Key), diff --git a/pkg/ingester/otlp/ingest_handler_test.go b/pkg/ingester/otlp/ingest_handler_test.go index b8b32c7eb6..7f76399758 100644 --- a/pkg/ingester/otlp/ingest_handler_test.go +++ b/pkg/ingester/otlp/ingest_handler_test.go @@ -298,6 +298,7 @@ func TestSampleAttributes(t *testing.T) { seriesLabelsMap[label.Name] = label.Value } assert.Equal(t, "", seriesLabelsMap["process"]) + assert.NotContains(t, seriesLabelsMap, "service.name") gp := new(googlev1.Profile) err = gp.UnmarshalVT(profiles[0].Series[0].Samples[0].RawProfile) @@ -445,8 +446,8 @@ func TestDifferentServiceNames(t *testing.T) { require.Equal(t, 2, len(profiles)) expectedStacks := map[string]string{ - "service-a": "(service.name = service-a) ||| serviceA_func2;serviceA_func1 100", - "service-b": "(service.name = service-b) ||| serviceB_func2;serviceB_func1 200", + "service-a": " ||| serviceA_func2;serviceA_func1 100", + "service-b": " ||| serviceB_func2;serviceB_func1 200", } // Verify service names, stacktraces, and profile metadata in the profiles @@ -459,6 +460,7 @@ func TestDifferentServiceNames(t *testing.T) { serviceName := seriesLabelsMap["service_name"] require.Contains(t, []string{"service-a", "service-b"}, serviceName) + assert.NotContains(t, seriesLabelsMap, "service.name") // Verify the profile contents gp := new(googlev1.Profile) @@ -484,6 +486,7 @@ func TestDifferentServiceNames(t *testing.T) { }) require.Equal(t, 1, len(ss)) assert.Equal(t, expectedStacks[serviceName], ss[0]) + assert.NotContains(t, ss[0], "service.name") } } From ad56611ba44ad7dc09dd07c498bb569451bee4df Mon Sep 17 00:00:00 2001 From: Tolya Korniltsev Date: Thu, 19 Dec 2024 13:48:19 +0700 Subject: [PATCH 6/7] make fmt --- pkg/ingester/otlp/convert.go | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/pkg/ingester/otlp/convert.go b/pkg/ingester/otlp/convert.go index 10842e96f1..825b314270 100644 --- a/pkg/ingester/otlp/convert.go +++ b/pkg/ingester/otlp/convert.go @@ -8,6 +8,8 @@ import ( otelProfile "github.com/grafana/pyroscope/api/otlp/profiles/v1experimental" ) +const serviceNameKey = "service.name" + // ConvertOtelToGoogle converts an OpenTelemetry profile to a Google profile. func ConvertOtelToGoogle(src *otelProfile.Profile) map[string]*googleProfile.Profile { svc2Profile := make(map[string]*profileBuilder) @@ -103,7 +105,7 @@ func (p *profileBuilder) addfunc(s string) uint64 { func serviceNameFromSample(p *otelProfile.Profile, sample *otelProfile.Sample) string { for _, attributeIndex := range sample.Attributes { attribute := p.AttributeTable[attributeIndex] - if attribute.Key == "service.name" { + if attribute.Key == serviceNameKey { return attribute.Value.GetStringValue() } } @@ -205,7 +207,7 @@ func (p *profileBuilder) convertSampleAttributesToLabelsBack(os *otelProfile.Sam gs.Label = make([]*googleProfile.Label, 0, len(os.Attributes)) for _, attribute := range os.Attributes { att := p.src.AttributeTable[attribute] - if att.Key == "service.name" { + if att.Key == serviceNameKey { continue } if att.Value.GetStringValue() != "" { From 0854bd021b63368b3c464c99609df050912a4e2e Mon Sep 17 00:00:00 2001 From: Tolya Korniltsev Date: Thu, 19 Dec 2024 18:09:24 +0700 Subject: [PATCH 7/7] review fixes --- pkg/ingester/otlp/ingest_handler_test.go | 41 +++++++++++++++--------- 1 file changed, 25 insertions(+), 16 deletions(-) diff --git a/pkg/ingester/otlp/ingest_handler_test.go b/pkg/ingester/otlp/ingest_handler_test.go index 7f76399758..4091bf447f 100644 --- a/pkg/ingester/otlp/ingest_handler_test.go +++ b/pkg/ingester/otlp/ingest_handler_test.go @@ -326,7 +326,6 @@ func TestDifferentServiceNames(t *testing.T) { }).Return(nil, nil) otlpb := new(otlpbuilder) - // Create mappings for both services otlpb.profile.Mapping = []*v1experimental.Mapping{{ MemoryStart: 0x1000, MemoryLimit: 0x2000, @@ -335,9 +334,12 @@ func TestDifferentServiceNames(t *testing.T) { MemoryStart: 0x2000, MemoryLimit: 0x3000, Filename: otlpb.addstr("service-b.so"), + }, { + MemoryStart: 0x4000, + MemoryLimit: 0x5000, + Filename: otlpb.addstr("service-c.so"), }} - // Create different locations for each service otlpb.profile.Location = []*v1experimental.Location{{ MappingIndex: 0, // service-a.so Address: 0x1100, @@ -366,9 +368,15 @@ func TestDifferentServiceNames(t *testing.T) { FunctionIndex: 3, Line: 40, }}, + }, { + MappingIndex: 2, // service-c.so + Address: 0xef0, + Line: []*v1experimental.Line{{ + FunctionIndex: 4, + Line: 50, + }}, }} - // Add functions otlpb.profile.Function = []*v1experimental.Function{{ Name: otlpb.addstr("serviceA_func1"), SystemName: otlpb.addstr("serviceA_func1"), @@ -385,11 +393,14 @@ func TestDifferentServiceNames(t *testing.T) { Name: otlpb.addstr("serviceB_func2"), SystemName: otlpb.addstr("serviceB_func2"), Filename: otlpb.addstr("service_b.go"), + }, { + Name: otlpb.addstr("serviceC_func3"), + SystemName: otlpb.addstr("serviceC_func3"), + Filename: otlpb.addstr("service_c.go"), }} - otlpb.profile.LocationIndices = []int64{0, 1, 2, 3} + otlpb.profile.LocationIndices = []int64{0, 1, 2, 3, 4, 4} - // Create two samples with different service.name attributes and different stacktraces otlpb.profile.Sample = []*v1experimental.Sample{{ LocationsStartIndex: 0, LocationsLength: 2, // Use first two locations @@ -397,12 +408,16 @@ func TestDifferentServiceNames(t *testing.T) { Attributes: []uint64{0}, }, { LocationsStartIndex: 2, - LocationsLength: 2, // Use last two locations + LocationsLength: 2, Value: []int64{200}, Attributes: []uint64{1}, + }, { + LocationsStartIndex: 4, + LocationsLength: 2, + Value: []int64{700}, + Attributes: []uint64{}, }} - // Set up the attribute table with different service names otlpb.profile.AttributeTable = []v1.KeyValue{{ Key: "service.name", Value: v1.AnyValue{ @@ -419,7 +434,6 @@ func TestDifferentServiceNames(t *testing.T) { }, }} - // Add sample types and period type otlpb.profile.SampleType = []*v1experimental.ValueType{{ Type: otlpb.addstr("cpu"), Unit: otlpb.addstr("nanoseconds"), @@ -442,15 +456,14 @@ func TestDifferentServiceNames(t *testing.T) { _, err := h.Export(context.Background(), req) require.NoError(t, err) - // We should have two separate profiles - require.Equal(t, 2, len(profiles)) + require.Equal(t, 3, len(profiles)) expectedStacks := map[string]string{ "service-a": " ||| serviceA_func2;serviceA_func1 100", "service-b": " ||| serviceB_func2;serviceB_func1 200", + "unknown": " ||| serviceC_func3;serviceC_func3 700", } - // Verify service names, stacktraces, and profile metadata in the profiles for _, p := range profiles { require.Equal(t, 1, len(p.Series)) seriesLabelsMap := make(map[string]string) @@ -459,26 +472,22 @@ func TestDifferentServiceNames(t *testing.T) { } serviceName := seriesLabelsMap["service_name"] - require.Contains(t, []string{"service-a", "service-b"}, serviceName) + require.Contains(t, []string{"service-a", "service-b", "unknown"}, serviceName) assert.NotContains(t, seriesLabelsMap, "service.name") - // Verify the profile contents gp := new(googlev1.Profile) err = gp.UnmarshalVT(p.Series[0].Samples[0].RawProfile) require.NoError(t, err) - // Verify sample types require.Equal(t, 1, len(gp.SampleType)) assert.Equal(t, "cpu", gp.StringTable[gp.SampleType[0].Type]) assert.Equal(t, "nanoseconds", gp.StringTable[gp.SampleType[0].Unit]) - // Verify period type require.NotNil(t, gp.PeriodType) assert.Equal(t, "cpu", gp.StringTable[gp.PeriodType.Type]) assert.Equal(t, "nanoseconds", gp.StringTable[gp.PeriodType.Unit]) assert.Equal(t, int64(10000000), gp.Period) - // Verify stacktraces ss := bench.StackCollapseProtoWithOptions(gp, bench.StackCollapseOptions{ ValueIdx: 0, Scale: 1,