From accc24556741b5530a3c26ee85fb53516274f67f Mon Sep 17 00:00:00 2001 From: Thomas Guettler Date: Fri, 2 Oct 2026 22:29:36 +0200 Subject: [PATCH] pkg/clickhouse: read the column, stop dropping function names A Go service's goroutine profile came back from a server running the ClickHouse backend with every frame unsymbolized, while scraping the very same /debug/pprof endpoint directly named all of them. The frame count survived ingestion; the names did not. The location encoders write a column between the line number and the hasFunction flag. decodeLineInfo did not read it, so it read the column's first byte as the flag. For pprof, which carries no column information, that byte is a uvarint zero, so the flag read as false and the whole function block -- name, system name, filename, start line -- was skipped. Nothing failed while this happened. The address, the mapping and the line number all decoded, the row was written, and the profile came back nameless. Measured on a live server before this change: 3,632,819 stored frames, every address and every line number decoded, zero function names. A non-zero column was worse than lossy. OTLP and Arrow locations carry real column numbers, and there the misread desynchronised the rest of the record: column 1 decoded a corrupted name ("\tmain.main"), and a larger start line walked the offset off the end of the buffer and panicked. The ClickHouse ingest path has no recover(), so that is a dead server rather than a failed request. Only profiles that arrive already symbolized lose names -- anything scraped from a Go /debug/pprof endpoint, and any other pre-symbolized upload. Those are also the ones the symbolizer cannot rescue afterwards, because they usually carry no build ID to look up, so the result reads like a debuginfo coverage gap rather than a decoder that could not parse what it was handed. profile.DecodeInto, the canonical decoder, reads line then column then the flag; every encoder in pkg/profile writes them in that order. decodeLineInfo was the only parser in the tree that disagreed. The tests drive the real encoders rather than hand-built blobs, because a blob written by hand would be written from whatever the decoder happened to do and so would agree with the bug it existed to catch. They cover the mapping and no-mapping branches, a line with no function, and non-zero columns through EncodeOtelLocation -- including multi-byte ones, since a zero column cannot tell a uvarint read apart from a bare offset++. --- pkg/clickhouse/ingester.go | 9 +++ pkg/clickhouse/ingester_test.go | 131 ++++++++++++++++++++++++++++++++ 2 files changed, 140 insertions(+) create mode 100644 pkg/clickhouse/ingester_test.go diff --git a/pkg/clickhouse/ingester.go b/pkg/clickhouse/ingester.go index 128a1a29fc9..4df702a8490 100644 --- a/pkg/clickhouse/ingester.go +++ b/pkg/clickhouse/ingester.go @@ -211,6 +211,15 @@ func decodeLineInfo(data []byte) LineInfo { offset += n info.LineNumber = int64(lineNum) + // Read the column. pprof carries no column information, so + // EncodePprofLocation writes a uvarint zero here -- a single 0x00 byte. + // Leaving it unread makes the hasFunction read below land on the column + // instead of the flag, where it is always false, which silently discards + // the function name, system name, filename and start line of every + // already-symbolized location. + _, n = varint.Uvarint(data[offset:]) + offset += n + hasFunction := data[offset] == 0x1 offset++ diff --git a/pkg/clickhouse/ingester_test.go b/pkg/clickhouse/ingester_test.go new file mode 100644 index 00000000000..2aa89119b6f --- /dev/null +++ b/pkg/clickhouse/ingester_test.go @@ -0,0 +1,131 @@ +// Copyright 2026 The Parca Authors +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package clickhouse + +import ( + "testing" + + "github.com/stretchr/testify/require" + pprofextended "go.opentelemetry.io/proto/otlp/profiles/v1development" + + pprofpb "github.com/parca-dev/parca/gen/proto/go/google/pprof" + "github.com/parca-dev/parca/pkg/profile" +) + +// decodeLineInfo must stay in step with the location encoders, which write a +// column between the line number and the hasFunction flag. +// +// Every encoder in pkg/profile agrees on that layout -- EncodePprofLocation, +// EncodeOtelLocation, EncodeArrowLocation and the normalizer's v2 encoder -- +// and so does the canonical decoder, profile.DecodeInto. A decoder that skips +// the column reads the column's own bytes as the flag, which costs the whole +// function block: name, system name, filename and start line. +// +// These drive the real encoders rather than hand-built blobs on purpose. A blob +// written by hand would be written from whatever the decoder happens to do, and +// so would agree with any bug it was supposed to catch. +func TestDecodeLineInfoRoundTripsTheEncodedLocation(t *testing.T) { + const ( + wantName = "bufio.(*Reader).Peek" + wantSys = "bufio.(*Reader).Peek" + wantFile = "/usr/local/go/src/bufio/bufio.go" + wantLine = 42 + wantStart = 10 + ) + + // pprof carries no column information, so EncodePprofLocation writes a + // uvarint zero -- a single 0x00 byte. The mapping block sits between the + // line count and the lines, so a decoder that is wrong about one can + // easily be right about the other; cover both. Values are non-zero so the + // uvarints are more than one byte each. + t.Run("pprof", func(t *testing.T) { + stringTable := []string{"", wantName, wantSys, wantFile, "build-id", "/bin/svc"} + funcs := []*pprofpb.Function{{Id: 1, Name: 1, SystemName: 2, Filename: 3, StartLine: wantStart}} + + for _, tc := range []struct { + name string + mapping *pprofpb.Mapping + }{ + {"with a mapping", &pprofpb.Mapping{ + Id: 1, BuildId: 4, Filename: 5, + MemoryStart: 0x1000, MemoryLimit: 0x2000, FileOffset: 8, + }}, + {"without a mapping", nil}, + } { + t.Run(tc.name, func(t *testing.T) { + loc := &pprofpb.Location{ + Id: 1, Address: 0xdeadbeef, + Line: []*pprofpb.Line{{FunctionId: 1, Line: wantLine}}, + } + if tc.mapping != nil { + loc.MappingId = tc.mapping.Id + } + + got := decodeLineInfo(profile.EncodePprofLocation(loc, tc.mapping, funcs, stringTable)) + + require.Equal(t, wantName, got.FunctionName, "the function name was discarded") + require.Equal(t, wantSys, got.FunctionSystemName) + require.Equal(t, wantFile, got.FunctionFilename) + require.EqualValues(t, wantLine, got.LineNumber) + require.EqualValues(t, wantStart, got.FunctionStartLine) + }) + } + }) + + // A zero column is a single 0x00 byte, which cannot tell a uvarint read + // apart from a bare offset++. OTLP locations carry real column numbers, so + // they are what pins the read down -- and a multi-byte column (>= 0x80) is + // what pins down that it is a *uvarint* read rather than a one-byte skip. + t.Run("otel with a non-zero column", func(t *testing.T) { + stringTable := []string{"", wantName, wantSys, wantFile, "build-id", "/bin/svc"} + funcs := []*pprofextended.Function{{ + NameStrindex: 1, SystemNameStrindex: 2, FilenameStrindex: 3, StartLine: wantStart, + }} + + for _, column := range []int64{1, 7, 300, 16384} { + t.Run("column", func(t *testing.T) { + loc := &pprofextended.Location{ + Address: 0xdeadbeef, + Lines: []*pprofextended.Line{{FunctionIndex: 1, Line: wantLine, Column: column}}, + } + + got := decodeLineInfo(profile.EncodeOtelLocation(nil, loc, nil, funcs, stringTable)) + + require.Equal(t, wantName, got.FunctionName, "column %d desynchronised the decoder", column) + require.Equal(t, wantSys, got.FunctionSystemName) + require.Equal(t, wantFile, got.FunctionFilename) + require.EqualValues(t, wantLine, got.LineNumber) + require.EqualValues(t, wantStart, got.FunctionStartLine) + }) + } + }) + + // A line with no function is a representable shape: the encoder writes the + // flag as 0x0 and no function block. The fields must come back zeroed -- + // which is the truth about this location, not a dropped name. + t.Run("line without a function", func(t *testing.T) { + loc := &pprofpb.Location{ + Id: 1, Address: 0xdeadbeef, + Line: []*pprofpb.Line{{FunctionId: 0, Line: wantLine}}, + } + + got := decodeLineInfo(profile.EncodePprofLocation(loc, nil, nil, []string{""})) + + require.EqualValues(t, wantLine, got.LineNumber) + require.Empty(t, got.FunctionName) + require.Empty(t, got.FunctionSystemName) + require.Empty(t, got.FunctionFilename) + require.Zero(t, got.FunctionStartLine) + }) +}