Skip to content

Commit

Permalink
feat(pyroscope.receive_http): ensure service_name label is set
Browse files Browse the repository at this point in the history
  • Loading branch information
marcsanmi committed Feb 10, 2025
1 parent 25a7089 commit 94f25b0
Show file tree
Hide file tree
Showing 3 changed files with 80 additions and 11 deletions.
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,8 @@ Main (unreleased)
- Reduce CPU usage of `loki.source.windowsevent` by up to 85% by updating the bookmark file every 10 seconds instead of after every event and by
optimizing the retrieval of the process name. (@wildum)

- Ensure consistent service_name label handling in `pyroscope.receive_http` to match Pyroscope's behavior. (@marcsanmi)

### Bugfixes

- Fix log rotation for Windows in `loki.source.file` by refactoring the component to use the runner pkg. This should also reduce CPU consumption when tailing a lot of files in a dynamic environment. (@wildum)
Expand Down
33 changes: 32 additions & 1 deletion internal/component/pyroscope/receive_http/receive_http.go
Original file line number Diff line number Diff line change
Expand Up @@ -190,7 +190,9 @@ func (c *Component) Push(ctx context.Context, req *connect.Request[pushv1.PushRe
for idx := range req.Msg.Series {
lb.Reset(nil)
setLabelBuilderFromAPI(lb, req.Msg.Series[idx].Labels)
err := appendable.Append(ctx, lb.Labels(), apiToAlloySamples(req.Msg.Series[idx].Samples))
// Ensure service_name label is set
lbls := ensureServiceName(lb.Labels())
err := appendable.Append(ctx, lbls, apiToAlloySamples(req.Msg.Series[idx].Samples))
if err != nil {
errs = errors.Join(
errs,
Expand Down Expand Up @@ -240,6 +242,9 @@ func (c *Component) handleIngest(w http.ResponseWriter, r *http.Request) {
}
}

// Ensure service_name label is set
lbls = ensureServiceName(lbls)

// Read the entire body into memory
// This matches how Append() handles profile data (as RawProfile),
// but means the entire profile will be held in memory
Expand Down Expand Up @@ -292,3 +297,29 @@ func (c *Component) shutdownServer() {
c.server = nil
}
}

const (
labelServiceName = "service_name"
labelAppName = "app_name"
)

func ensureServiceName(lbls labels.Labels) labels.Labels {
// If service_name is already present, add app_name if not exists
if lbls.Has(labelServiceName) {
if appName := lbls.Get("__name__"); appName != "" && !lbls.Has(labelAppName) {
builder := labels.NewBuilder(lbls)
builder.Set(labelAppName, appName)
return builder.Labels()
}
return lbls
}

// If no service_name, use __name__ value for service_name
if appName := lbls.Get("__name__"); appName != "" {
builder := labels.NewBuilder(lbls)
builder.Set(labelServiceName, appName)
return builder.Labels()
}

return lbls
}
56 changes: 46 additions & 10 deletions internal/component/pyroscope/receive_http/receive_http_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,10 @@ func TestForwardsProfilesIngest(t *testing.T) {
appendableErrors: []error{nil, nil},
expectedStatus: http.StatusOK,
expectedForwards: 2,
expectedLabels: map[string]string{
"__name__": "test_app",
"service_name": "test_app",
},
},
{
name: "Large profile with custom headers",
Expand All @@ -67,6 +71,10 @@ func TestForwardsProfilesIngest(t *testing.T) {
appendableErrors: []error{nil},
expectedStatus: http.StatusOK,
expectedForwards: 1,
expectedLabels: map[string]string{
"__name__": "test_app",
"service_name": "test_app",
},
},
{
name: "Invalid method",
Expand Down Expand Up @@ -111,6 +119,10 @@ func TestForwardsProfilesIngest(t *testing.T) {
appendableErrors: []error{fmt.Errorf("error1"), fmt.Errorf("error2")},
expectedStatus: http.StatusInternalServerError,
expectedForwards: 2,
expectedLabels: map[string]string{
"__name__": "test_app",
"service_name": "test_app",
},
},
{
name: "One appendable fails, one succeeds",
Expand All @@ -122,6 +134,10 @@ func TestForwardsProfilesIngest(t *testing.T) {
appendableErrors: []error{fmt.Errorf("error"), nil},
expectedStatus: http.StatusInternalServerError,
expectedForwards: 2,
expectedLabels: map[string]string{
"__name__": "test_app",
"service_name": "test_app",
},
},
{
name: "Valid labels are parsed and forwarded",
Expand All @@ -134,9 +150,10 @@ func TestForwardsProfilesIngest(t *testing.T) {
expectedStatus: http.StatusOK,
expectedForwards: 2,
expectedLabels: map[string]string{
"__name__": "test.app",
"env": "prod",
"region": "us-east",
"__name__": "test.app",
"service_name": "test.app",
"env": "prod",
"region": "us-east",
},
},
{
Expand All @@ -150,7 +167,24 @@ func TestForwardsProfilesIngest(t *testing.T) {
expectedStatus: http.StatusOK,
expectedForwards: 2,
expectedLabels: map[string]string{
"__name__": "test.app", // Only __name__ is preserved
"__name__": "test.app",
"service_name": "test.app",
},
},
{
name: "existing service_name sets app_name from __name__",
profileSize: 1024,
method: "POST",
path: "/ingest",
queryParams: "name=test.app{service_name=my-service}",
headers: map[string]string{"Content-Type": "application/octet-stream"},
appendableErrors: []error{nil},
expectedStatus: http.StatusOK,
expectedForwards: 1,
expectedLabels: map[string]string{
"__name__": "test.app",
"service_name": "my-service",
"app_name": "test.app",
},
},
}
Expand Down Expand Up @@ -333,12 +367,14 @@ func verifyForwardedProfiles(
testApp, ok := app.(*testAppender)
require.True(t, ok, "Appendable is not a testAppender")

// Verify labels if name parameter exists and is valid
if nameParam := testApp.lastProfile.URL.Query().Get("name"); nameParam != "" {
ls, err := labelset.Parse(nameParam)
if err == nil {
require.Equal(t, ls.Labels(), testApp.lastProfile.Labels.Map(),
"Labels mismatch for appendable %d", i)
// Skip name parameter label check if we're testing service_name behavior
if expectedLabels == nil || expectedLabels["service_name"] == "" {
if nameParam := testApp.lastProfile.URL.Query().Get("name"); nameParam != "" {
ls, err := labelset.Parse(nameParam)
if err == nil {
require.Equal(t, ls.Labels(), testApp.lastProfile.Labels.Map(),
"Labels mismatch for appendable %d", i)
}
}
}

Expand Down

0 comments on commit 94f25b0

Please sign in to comment.