fix: Return modified labels after validation sanitization

[jake-kramer]

This fixes a bug where the logic in handleSanitizedLabel was not properly executed. The code would modify the slice itself, but this new modified slice was never returned up the call stack.

[marcsanmi]

AFAIU, the modified length wasn't being propagated back to the callers, was it?

LGTM

[marcsanmi]

cc @simonswine you might want to take a look as well.

[jake-kramer]

AFAIU, the modified length wasn't being propagated back to the callers, was it?

LGTM

Yes exactly, here:

pyroscope/pkg/validation/validate.go

Lines 207 to 228 in 2e303da

	if found {
	if newSlice[insertIdx].Value == newLabel.Value {
	// Same name and value we are done and can just return newSlice
	return newSlice, origIdx, nil
	} else {
	// Same name, different value - error
	return nil, 0, NewErrorf(DuplicateLabelNames,
	DuplicateLabelNamesAfterSanitizationErrorMsg,
	phlaremodel.LabelPairsString(ls), newName, origName)
	}
	}
	// Insert the new label at correct position
	newSlice = slices.Insert(newSlice, insertIdx, newLabel)
	finalIdx := insertIdx
	if insertIdx >= origIdx {
	finalIdx = origIdx
	}
	copy(ls, newSlice)
	return ls[:len(newSlice)], finalIdx, nil

[simonswine]

LGTM

Thanks for catching and fixing this, it is amazing how many bugs we have found in this piece of code.

I think ew can merge this as is, but probably we should rename ValidateLabels to SanitizeLabels, as its name doesn't suggest, it is a mutation operation.

[jake-kramer]

It will soon be just validation since the sanitization logic will be removed with the utf8 label name work