Skip to content

[exporter/elasticsearch] Add downsampling for profiling events #37893

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.

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

Conversation

rockdaboot
Copy link
Contributor

The stratified downsampling is essential to reduce the query latency for profiling data.

@rockdaboot rockdaboot changed the title [exporter/elasticsearch] Add downsampling for profiling events [chore][exporter/elasticsearch] Add downsampling for profiling events Feb 13, 2025
@rockdaboot rockdaboot requested a review from carsonip February 18, 2025 16:44
Copy link
Contributor

@carsonip carsonip left a comment

Choose a reason for hiding this comment

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

it just missed the last release (my fault!), we might need a changelog for it.

Comment on lines +25 to +31
pushDataAsJSON := func(data any, id, index string) error {
c, err := toJSON(data)
if err != nil {
return err
}
return pushData(c, id, index)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this be moved to be own function definition to help make it easier to debug the stack output in the event of an error?

Copy link
Member

Choose a reason for hiding this comment

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

This isn't so trivial, because the pushData method is local to SerializeProfiles, and we can't pass it as an argument (to use it with no arguments in IndexDownsampledEvent.

This would work. Is that what you expect?

diff --git a/exporter/elasticsearchexporter/internal/serializer/otelserializer/profile.go b/exporter/elasticsearchexporter/internal/serializer/otelserializer/profile.go
index eb324d8f3d..0398af40e9 100644
--- a/exporter/elasticsearchexporter/internal/serializer/otelserializer/profile.go
+++ b/exporter/elasticsearchexporter/internal/serializer/otelserializer/profile.go
@@ -20,16 +20,10 @@ const (
 	ExecutablesIndex = "profiling-executables"
 )
 
-// SerializeProfile serializes a profile and calls the `pushData` callback for each generated document.
-func SerializeProfile(resource pcommon.Resource, scope pcommon.InstrumentationScope, profile pprofile.Profile, pushData func(*bytes.Buffer, string, string) error) error {
-	pushDataAsJSON := func(data any, id, index string) error {
-		c, err := toJSON(data)
-		if err != nil {
-			return err
-		}
-		return pushData(c, id, index)
-	}
+type dataPusher func(*bytes.Buffer, string, string) error
 
+// SerializeProfile serializes a profile and calls the `pushData` callback for each generated document.
+func SerializeProfile(resource pcommon.Resource, scope pcommon.InstrumentationScope, profile pprofile.Profile, pushData dataPusher) error {
 	data, err := serializeprofiles.Transform(resource, scope, profile)
 	if err != nil {
 		return err
@@ -39,28 +33,28 @@ func SerializeProfile(resource pcommon.Resource, scope pcommon.InstrumentationSc
 		event := payload.StackTraceEvent
 
 		if event.StackTraceID != "" {
-			if err = pushDataAsJSON(event, "", AllEventsIndex); err != nil {
+			if err = pushDataAsJSON(pushData)(event, "", AllEventsIndex); err != nil {
 				return err
 			}
-			if err = serializeprofiles.IndexDownsampledEvent(event, pushDataAsJSON); err != nil {
+			if err = serializeprofiles.IndexDownsampledEvent(event, pushDataAsJSON(pushData)); err != nil {
 				return err
 			}
 		}
 
 		if payload.StackTrace.DocID != "" {
-			if err = pushDataAsJSON(payload.StackTrace, payload.StackTrace.DocID, StackTraceIndex); err != nil {
+			if err = pushDataAsJSON(pushData)(payload.StackTrace, payload.StackTrace.DocID, StackTraceIndex); err != nil {
 				return err
 			}
 		}
 
 		for _, stackFrame := range payload.StackFrames {
-			if err = pushDataAsJSON(stackFrame, stackFrame.DocID, StackFrameIndex); err != nil {
+			if err = pushDataAsJSON(pushData)(stackFrame, stackFrame.DocID, StackFrameIndex); err != nil {
 				return err
 			}
 		}
 
 		for _, executable := range payload.Executables {
-			if err = pushDataAsJSON(executable, executable.DocID, ExecutablesIndex); err != nil {
+			if err = pushDataAsJSON(pushData)(executable, executable.DocID, ExecutablesIndex); err != nil {
 				return err
 			}
 		}
@@ -76,3 +70,13 @@ func toJSON(d any) (*bytes.Buffer, error) {
 
 	return bytes.NewBuffer(c), nil
 }
+
+func pushDataAsJSON(pushData dataPusher) func(any, string, string) error {
+	return func(data any, id, index string) error {
+		c, err := toJSON(data)
+		if err != nil {
+			return err
+		}
+		return pushData(c, id, index)
+	}
+}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@MovieStoreGuy Any opinion on what @dmathieu says or different suggestions? I'd rather keep the code as is, as I can't see why developers should have an issue with stacktraces here.

@rockdaboot rockdaboot changed the title [chore][exporter/elasticsearch] Add downsampling for profiling events [exporter/elasticsearch] Add downsampling for profiling events Feb 28, 2025
@rockdaboot
Copy link
Contributor Author

Is there anything that needs to done to get this PR merged?

@andrzej-stencel andrzej-stencel merged commit dcf7b2a into open-telemetry:main Mar 11, 2025
157 checks passed
@github-actions github-actions bot added this to the next release milestone Mar 11, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants