Skip to content

Commit

Permalink
fix: otlp pprof fixes (#3741)
Browse files Browse the repository at this point in the history
    Do not append sample labels to series labels (imagine two samples with process=firefox and process=chrome, these attributes should not be appended to Push request series labels. Instead add them as sample labels.
    Fix function name for unsymbolized functions, previously it was incorrectly using only one function per mapping. Now we create multiple (a lot of them actually) functions with format libfoo.so 0x...
    Remove debugging pprof dump to fs
  • Loading branch information
korniltsev authored Dec 6, 2024
1 parent 749fbab commit 1886f2c
Show file tree
Hide file tree
Showing 6 changed files with 342 additions and 150 deletions.
3 changes: 3 additions & 0 deletions .mockery.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,9 @@ packages:
github.com/grafana/pyroscope/pkg/frontend:
interfaces:
Limits:
github.com/grafana/pyroscope/pkg/ingester/otlp:
interfaces:
PushService:
github.com/grafana/pyroscope/pkg/experiment/metastore/discovery:
interfaces:
Discovery:
Expand Down
64 changes: 41 additions & 23 deletions pkg/ingester/otlp/convert.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,15 @@ func ConvertOtelToGoogle(src *otelProfile.Profile) *googleProfile.Profile {
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("")

if dst.TimeNanos == 0 {
dst.TimeNanos = time.Now().UnixNano()
Expand All @@ -48,49 +57,44 @@ func ConvertOtelToGoogle(src *otelProfile.Profile) *googleProfile.Profile {
gf.Id = uint64(i + 1)
dst.Function = append(dst.Function, gf)
}
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]
}

functionOffset := uint64(len(dst.Function)) + 1
dst.Location = []*googleProfile.Location{}
locationMappingIndexAddressMap := make(map[uint64]uint64)
// 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: loc.MappingIndex + functionOffset,
FunctionId: addfunc(fmt.Sprintf("%s 0x%x", src.StringTable[m.Filename], loc.Address)),
})
}
dst.Location = append(dst.Location, gl)
locationMappingIndexAddressMap[loc.MappingIndex] = loc.Address
}

for _, m := range src.Mapping {
address := locationMappingIndexAddressMap[m.Id]
addressStr := fmt.Sprintf("%s 0x%x", dst.StringTable[m.Filename], address)
dst.StringTable = append(dst.StringTable, addressStr)
// i == 0 function_id = functionOffset
id := uint64(len(dst.Function)) + 1
dst.Function = append(dst.Function, &googleProfile.Function{
Id: id,
Name: int64(len(dst.StringTable) - 1),
})
}

// Convert samples
for _, sample := range src.Sample {
gs := convertSampleBack(sample, src.LocationIndices)
gs := convertSampleBack(src, sample, src.LocationIndices, addstr)
dst.Sample = append(dst.Sample, gs)
}

if len(dst.SampleType) == 0 {
dst.StringTable = append(dst.StringTable, "samples")
dst.StringTable = append(dst.StringTable, "ms")
dst.SampleType = []*googleProfile.ValueType{{
Type: int64(len(dst.StringTable) - 2),
Unit: int64(len(dst.StringTable) - 1),
Type: addstr("samples"),
Unit: addstr("ms"),
}}
dst.DefaultSampleType = int64(len(dst.StringTable) - 2)
dst.DefaultSampleType = addstr("samples")
}

return dst
Expand Down Expand Up @@ -147,14 +151,15 @@ func convertFunctionBack(of *otelProfile.Function) *googleProfile.Function {
}
}

func convertSampleBack(os *otelProfile.Sample, locationIndexes []int64) *googleProfile.Sample {
func convertSampleBack(p *otelProfile.Profile, os *otelProfile.Sample, locationIndexes []int64, addstr func(s string) int64) *googleProfile.Sample {
gs := &googleProfile.Sample{
Value: os.Value,
}

if len(gs.Value) == 0 {
gs.Value = []int64{int64(len(os.TimestampsUnixNano))}
}
convertSampleAttributesToLabelsBack(p, os, gs, addstr)

for i := os.LocationsStartIndex; i < os.LocationsStartIndex+os.LocationsLength; i++ {
gs.LocationId = append(gs.LocationId, uint64(locationIndexes[i]+1))
Expand All @@ -163,6 +168,19 @@ func convertSampleBack(os *otelProfile.Sample, locationIndexes []int64) *googleP
return gs
}

func convertSampleAttributesToLabelsBack(p *otelProfile.Profile, os *otelProfile.Sample, gs *googleProfile.Sample, addstr func(s string) int64) {
gs.Label = make([]*googleProfile.Label, 0, len(os.Attributes))
for _, attribute := range os.Attributes {
att := p.AttributeTable[attribute]
if att.Value.GetStringValue() != "" {
gs.Label = append(gs.Label, &googleProfile.Label{
Key: addstr(att.Key),
Str: 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))
Expand Down
45 changes: 0 additions & 45 deletions pkg/ingester/otlp/ingest_handler.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ import (
"context"
"fmt"
"net/http"
"os"
"strings"

"connectrpc.com/connect"
Expand All @@ -18,7 +17,6 @@ import (
typesv1 "github.com/grafana/pyroscope/api/gen/proto/go/types/v1"
pprofileotlp "github.com/grafana/pyroscope/api/otlp/collector/profiles/v1experimental"
v1 "github.com/grafana/pyroscope/api/otlp/common/v1"
"github.com/grafana/pyroscope/api/otlp/profiles/v1experimental"
"github.com/grafana/pyroscope/pkg/tenant"
)

Expand Down Expand Up @@ -111,16 +109,11 @@ func (h *ingestHandler) Export(ctx context.Context, er *pprofileotlp.ExportProfi
// Add profile attributes
labels = appendAttributesUnique(labels, p.GetAttributes(), processedKeys)

// Add profile-specific attributes from samples/attributetable
labels = appendProfileLabels(labels, p.Profile, processedKeys)

pprofBytes, err := OprofToPprof(p.Profile)
if err != nil {
return &pprofileotlp.ExportProfilesServiceResponse{}, fmt.Errorf("failed to convert from OTLP to legacy pprof: %w", err)
}

_ = os.WriteFile(".tmp/elastic.pprof", pprofBytes, 0644)

req := &pushv1.PushRequest{
Series: []*pushv1.RawProfileSeries{
{
Expand Down Expand Up @@ -199,41 +192,3 @@ func appendAttributesUnique(labels []*typesv1.LabelPair, attrs []v1.KeyValue, pr
}
return labels
}

func appendProfileLabels(labels []*typesv1.LabelPair, profile *v1experimental.Profile, processedKeys map[string]bool) []*typesv1.LabelPair {
if profile == nil {
return labels
}

// Create mapping of attribute indices to their values
attrMap := make(map[uint64]v1.AnyValue)
for i, attr := range profile.GetAttributeTable() {
val := attr.GetValue()
if val.GetValue() != nil {
attrMap[uint64(i)] = val
}
}

// Process only attributes referenced in samples
for _, sample := range profile.Sample {
for _, attrIdx := range sample.GetAttributes() {
attr := profile.AttributeTable[attrIdx]
// Skip if we've already processed this key at any level
if processedKeys[attr.Key] {
continue
}

if value, exists := attrMap[attrIdx]; exists {
if sv := value.GetStringValue(); sv != "" {
labels = append(labels, &typesv1.LabelPair{
Name: attr.Key,
Value: sv,
})
processedKeys[attr.Key] = true
}
}
}
}

return labels
}
Loading

0 comments on commit 1886f2c

Please sign in to comment.