fix: export of custom pprof labels #2667
Conversation
pkg/parcacol/querier.go
Outdated
| @@ -102,7 +107,8 @@ func (q *Querier) Labels( | |||
| } | |||
|
|
|||
| val := stringCol.Value(i) | |||
| seen[strings.TrimPrefix(val, "labels.")] = struct{}{} | |||
| l := strings.TrimPrefix(strings.TrimPrefix(val, ColumnLabelsPrefix), ColumnPprofLabelsPrefix) | |||
There was a problem hiding this comment.
based on the prefix we can tell what kind of profile it is and return that to the frontend, and only the frontend will unify it into a single list
There was a problem hiding this comment.
I don't quite understand 😄
I assumed that since we removed the prefix for labels. we should do the same irrespective of where the prefix comes from.
Do you mean we should only trim the labels. prefix and the frontend is going to take care of removing the pprof_labels. prefix?
There was a problem hiding this comment.
I mean that the response should have one list that is pprof_labels, and one that is regular labels. That way the frontend will be able to tell whether a label is a profile label or a regular one, but to the user it will only show a single list.
The reason we need this knowledge in the frontend is to be able to tell the LabelValues request, whether a label is a pprof label or a regular label.
pkg/parcacol/querier.go
Outdated
| if err != nil { | ||
| return nil, err | ||
| } | ||
| return logicalplan.Or(labelExpr, pprofLabelExpr), nil |
There was a problem hiding this comment.
Like I said on Discord, I'm not sure this is quite correct, we actually need to say something like "label.a exists and is equal to "b" and pprof_labels.a does not exist OR pprof.labels_a exists and is equal to "b" and labels.a does not exist"
Unfortunately, frostdb doesn't support this yet.
I think we can leave this part for last though, we might have it implemented by the time we fix all the other things! :)
|
Created polarsignals/frostdb#394 and added it to the team's backlog so we'll hopefully have the last missing piece for this soon! Thanks for bearing with us! |
|
polarsignals/frostdb#394 is implemented so we can now build the queries we want to here! 🥳 |
pkg/parcacol/querier.go
Outdated
| @@ -102,7 +107,8 @@ func (q *Querier) Labels( | |||
| } | |||
|
|
|||
| val := stringCol.Value(i) | |||
| seen[strings.TrimPrefix(val, "labels.")] = struct{}{} | |||
| l := strings.TrimPrefix(strings.TrimPrefix(val, ColumnLabelsPrefix), ColumnPprofLabelsPrefix) | |||
There was a problem hiding this comment.
I mean that the response should have one list that is pprof_labels, and one that is regular labels. That way the frontend will be able to tell whether a label is a profile label or a regular one, but to the user it will only show a single list.
The reason we need this knowledge in the frontend is to be able to tell the LabelValues request, whether a label is a pprof label or a regular label.
teleivo
changed the title
Normalization of labels is done twice. Label normalization before ingest was adding the collision prefix 'exported_' even if no collision was present.
| samples: []*pprofpb.Sample{ | ||
| { | ||
| Label: []*pprofpb.Label{{ | ||
| Key: 3, |
There was a problem hiding this comment.
@brancz I just want to make sure I get this right 😅 I added some missing tests based on the ones that were already here. The order in colliding labels in descending order refers to the order of the []*pprofpb.Label right? So the label.Key in descending order is method, instance in this case and reversed in the ascending case.
|
Just rebased and tested this, and it works flawlessly. I'd be happy to merge this! |
work in progress to fix #2657
pkg/parcacol/normalizer_test.gosetup data as it did not seem to reflect reality. ThestringTabledoes not contain the takenLabels key/values when running parca.stringTableonly contains strings from the profile. In case of a collision, the colliding key will be in thestringTabledue to it being in the profile. In such a case it is not prefixed withexported_as this is done by parca.