Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: otlp pprof fixes #3741

Merged
merged 5 commits into from
Dec 6, 2024
Merged

fix: otlp pprof fixes #3741

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
dda35cf
tmp
korniltsev Dec 5, 2024
c24dfbd
tests
korniltsev Dec 5, 2024
4c0f7bc
fix
korniltsev Dec 5, 2024
1317331
fmt
korniltsev Dec 5, 2024
9d6cb45
make fmt
korniltsev Dec 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions .mockery.yaml
View file
Open in desktop
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
View file
Open in desktop
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("")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this needed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is better to do it explicitly for https://github.com/google/pprof/blob/1e3fdc7de4679692c8c3bafdb87274f45090c73a/proto/profile.proto#L66


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 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So my understanding is that we only create those "placeholder" functions currently, as our query path relies on them to deduplicate.

I think a comment would be nice explaining that.

Maybe something worth considering doing later in the write path. Because if I sent currently Google pprofs without symbols I have the exact same problem. Or am I missing something 😂

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm... I didn't test it, but it should be the same if we happen to receive pprof without symbols (lines) 😅

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think frontend or read path just skips these locations.

image

vs

image

I think it is mostly needed to show the correct shape of the flamegraph - display that there were some frames we just could not name them

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
View file
Open in desktop
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
Loading