diff --git a/pkg/normalizer/arrow_v2.go b/pkg/normalizer/arrow_v2.go index b82fd1e5f36..a676a8b1752 100644 --- a/pkg/normalizer/arrow_v2.go +++ b/pkg/normalizer/arrow_v2.go @@ -520,7 +520,9 @@ func encodeStacktraceListViewV2(mem memory.Allocator, stacktrace *array.ListView // if hasFunction: uvarint(startLine), string(name), string(systemName), string(filename) // // v2 has no mapping_start/limit/offset, so they are written as zero. v2 has no -// separate function name vs. system_name, so name is written as the empty string. +// separate function name vs. system_name, so system_name is written as both; +// readers that only look at the name (flame graph, table) would otherwise see +// every pre-symbolized frame (e.g. kernel frames) as unnamed. func encodeV2Location(locIdx int, r *v2LocationReader) []byte { address := r.address.Value(locIdx) @@ -572,9 +574,10 @@ func encodeV2Location(locIdx int, r *v2LocationReader) []byte { if !ok { continue } + systemName := v2FunctionSystemName(r, fIdx) size += uvarintSize(r.startLine.Value(fIdx)) - size += sizeOfString("") // function name (not present in v2) - size += sizeOfString(v2FunctionSystemName(r, fIdx)) + size += sizeOfString(systemName) // name + size += sizeOfString(systemName) size += sizeOfString(v2FunctionFilename(r, fIdx)) } } @@ -613,9 +616,10 @@ func encodeV2Location(locIdx int, r *v2LocationReader) []byte { } buf[offset] = 0x1 offset++ + systemName := v2FunctionSystemName(r, fIdx) offset += binary.PutUvarint(buf[offset:], r.startLine.Value(fIdx)) - offset = writeStringV2(buf, offset, "") - offset = writeStringV2(buf, offset, v2FunctionSystemName(r, fIdx)) + offset = writeStringV2(buf, offset, systemName) + offset = writeStringV2(buf, offset, systemName) offset = writeStringV2(buf, offset, v2FunctionFilename(r, fIdx)) } } diff --git a/pkg/normalizer/arrow_v2_test.go b/pkg/normalizer/arrow_v2_test.go index d3f9cd4cd9e..20ed2be7a77 100644 --- a/pkg/normalizer/arrow_v2_test.go +++ b/pkg/normalizer/arrow_v2_test.go @@ -14,6 +14,7 @@ package normalizer import ( + "bytes" "context" "testing" @@ -70,6 +71,13 @@ func TestAddSampleRecordV2(t *testing.T) { require.NotEmpty(t, schema.FieldIndices(name), "expected internal record to have column %q", name) } + // v2 only carries system_name; it must also be written as the function + // name, which is what the flame graph and table read. + stacks := out.Column(schema.FieldIndices(profile.ColumnStacktrace)[0]).(*array.List) + locs := stacks.ListValues().(*array.Dictionary) + encoded := locs.Dictionary().(*array.Binary).Value(locs.GetValueIndex(0)) + require.Equal(t, 2, bytes.Count(encoded, []byte("do_thing")), "expected name and system_name to both be do_thing") + // Verify the timestamp ms / time_nanos split. timestampIdx := schema.FieldIndices(profile.ColumnTimestamp)[0] timeNanosIdx := schema.FieldIndices(profile.ColumnTimeNanos)[0] diff --git a/pkg/profile/decode.go b/pkg/profile/decode.go index e7bfde9e5fd..e330a149f2a 100644 --- a/pkg/profile/decode.go +++ b/pkg/profile/decode.go @@ -159,6 +159,15 @@ func DecodeInto(lw LocationsWriter, data []byte, demangler Demangler) (DecodeRes name, n := decodeString(data[offset:]) offset += n + systemName, n := decodeString(data[offset:]) + offset += n + + // Data written by the v2 ingest path before it populated the + // name only carries system_name. + if len(name) == 0 { + name = systemName + } + if demangler != nil { name = []byte(demangler.Demangle(name)) } @@ -167,9 +176,6 @@ func DecodeInto(lw LocationsWriter, data []byte, demangler Demangler) (DecodeRes return DecodeResult{}, fmt.Errorf("append function name: %w", err) } - systemName, n := decodeString(data[offset:]) - offset += n - if err := lw.FunctionSystemName.Append(systemName); err != nil { return DecodeResult{}, fmt.Errorf("append function system name: %w", err) } @@ -257,7 +263,11 @@ func DecodeFunctionName(data []byte) ([]byte, error) { _, n = varint.Uvarint(data[offset:]) offset += n - name, _ := decodeString(data[offset:]) + name, n := decodeString(data[offset:]) + if len(name) == 0 { + // Fall back to system_name, matching DecodeInto. + name, _ = decodeString(data[offset+n:]) + } return name, nil } } diff --git a/pkg/profile/encode_test.go b/pkg/profile/encode_test.go index 57c578cb890..4a7cd39932c 100644 --- a/pkg/profile/encode_test.go +++ b/pkg/profile/encode_test.go @@ -16,7 +16,9 @@ package profile import ( "testing" + "github.com/apache/arrow-go/v18/arrow/array" "github.com/apache/arrow-go/v18/arrow/memory" + "github.com/stretchr/testify/require" pprofpb "github.com/parca-dev/parca/gen/proto/go/google/pprof" ) @@ -72,3 +74,29 @@ func TestEncodeDecode(t *testing.T) { }) } } + +// Locations written by the v2 ingest path before it populated the function +// name (e.g. agent-symbolized kernel frames) only carry a system name. +func TestDecodeFallsBackToSystemName(t *testing.T) { + t.Parallel() + + buf := EncodePprofLocation( + &pprofpb.Location{Line: []*pprofpb.Line{{Line: 1, FunctionId: 1}}}, + nil, + []*pprofpb.Function{{SystemName: 1, Filename: 2}}, + []string{"", "do_syscall_64", "vmlinux"}, + ) + + name, err := DecodeFunctionName(buf) + require.NoError(t, err) + require.Equal(t, "do_syscall_64", string(name)) + + lw := NewLocationsWriter(memory.DefaultAllocator) + defer lw.RecordBuilder.Release() + _, err = DecodeInto(lw, buf, nil) + require.NoError(t, err) + + arr := lw.FunctionName.NewArray().(*array.Dictionary) + defer arr.Release() + require.Equal(t, "do_syscall_64", arr.Dictionary().(*array.Binary).ValueString(arr.GetValueIndex(0))) +}