[Profiling] Aggregate flamegraph by process name and thread name #119115

rockdaboot · 2024-12-19T16:47:40Z

This PR adds aggregation by process name and by thread name to the Universal Profiling flamegraph view.

The profiling data model has been updated to fit for the new task, so some non-trivial amount of code had to be changed.

TBD: fix and improve tests

This PR works best together with this Kibana PR.

Before

After

elasticsearchmachine · 2024-12-19T16:48:05Z

Pinging @elastic/obs-knowledge-team (Team:obs-knowledge)

elasticsearchmachine · 2024-12-19T16:48:06Z

Pinging @elastic/obs-ux-infra_services-team (Team:obs-ux-infra_services)

...ore/template-resources/src/main/resources/profiling/component-template/profiling-events.json

Co-authored-by: Jake Landis <[email protected]>

christos68k

Added some suggestions to avoid confusion if/when someone revisits source code later in time. LGTM otherwise (though I don't really have knowledge of the internals here, so another review from a domain expert would be nice)

...ing/src/main/java/org/elasticsearch/xpack/profiling/action/TransportGetFlamegraphAction.java

...ng/src/main/java/org/elasticsearch/xpack/profiling/action/TransportGetStackTracesAction.java

…, root) (#204977) ## Summary This PR is a pre-requisite for adding aggregation by process name and by thread name to the Universal Profiling flamegraph view. It adds three artificial node types to the flamegraph including color codes. As a side-effect, the root node now has its own color code. Previously, it (accidentally) used the color code of "unknown" type frames. The PR is backwards compatible, so it doesn't change anything in the UI when connecting with current Elasticsearch. As soon as [the PR for ES](elastic/elasticsearch#119115) is merged, the new aggregations show up.

…, root) (elastic#204977) ## Summary This PR is a pre-requisite for adding aggregation by process name and by thread name to the Universal Profiling flamegraph view. It adds three artificial node types to the flamegraph including color codes. As a side-effect, the root node now has its own color code. Previously, it (accidentally) used the color code of "unknown" type frames. The PR is backwards compatible, so it doesn't change anything in the UI when connecting with current Elasticsearch. As soon as [the PR for ES](elastic/elasticsearch#119115) is merged, the new aggregations show up.

danielmitterdorfer · 2025-01-10T13:10:59Z

x-pack/plugin/build.gradle

@@ -96,5 +96,7 @@ tasks.named("yamlRestCompatTestTransform").configure({ task ->
  task.skipTest("esql/180_match_operator/match with non text field", "Match operator can now be used on non-text fields")
  task.skipTest("esql/180_match_operator/match with functions", "Error message changed")
  task.skipTest("esql/40_unsupported_types/semantic_text declared in mapping", "The semantic text field format changed")
+  task.replaceValueInMatch("Size", 49, "Test flamegraph from profiling-events")


Why is this instruction needed here?

I think it is because comment

...ore/template-resources/src/main/resources/profiling/component-template/profiling-events.json

danielmitterdorfer · 2025-01-10T13:15:20Z

...profiling/src/main/java/org/elasticsearch/xpack/profiling/action/GetStackTracesResponse.java

            ),
-            Iterators.single((b, p) -> b.field("total_frames", totalFrames)),
-            Iterators.single((b, p) -> b.field("sampling_rate", samplingRate)),
+            Iterators.single((b, p) -> b.field("total_frames", totalFrames).field("sampling_rate", samplingRate).endObject())


This looks like an unrelated cleanup / optimization?

Yes, it is an unrelated change. It has been suggested by Armin Braun in a chat. See comment

danielmitterdorfer · 2025-01-10T13:16:13Z

...profiling/src/main/java/org/elasticsearch/xpack/profiling/action/GetStackTracesResponse.java

@@ -100,23 +100,24 @@ public Iterator<? extends ToXContent> toXContentChunked(ToXContent.Params params
            optional(
                "stack_trace_events",
                stackTraceEvents,
-                (n, v) -> ChunkedToXContentHelper.map(n, v, entry -> (b, p) -> b.field(entry.getKey(), entry.getValue().count))
+                (n, v) -> ChunkedToXContentHelper.wrapWithObject(


Doesn't this change the structure of the output (backwards-compatibility)?

Do you see any issues with this in Kibana (I didn't find any issue so far). BWC tests seem to run fine as well.

...ing/src/main/java/org/elasticsearch/xpack/profiling/action/TransportGetFlamegraphAction.java

danielmitterdorfer · 2025-01-10T13:31:32Z

...ng/src/main/java/org/elasticsearch/xpack/profiling/action/TransportGetStackTracesAction.java


                // aggregation
-                Map<String, TraceEvent> stackTraceEvents = new TreeMap<>();
+                Map<TraceEventID, TraceEvent> stackTraceEvents = new HashMap<>();


Changing this might have a performance impact in the K/V lookups. Is there a reason that you've changed this? It is possible to implement Comparable for TraceEventID to keep using TreeMap.

Update: I see that you sort in place below when extracting the ids so that should be fine.

...ng/src/main/java/org/elasticsearch/xpack/profiling/action/TransportGetStackTracesAction.java

.../src/main/java/org/elasticsearch/xpack/profiling/action/TransportGetTopNFunctionsAction.java

…, root) (elastic#204977) ## Summary This PR is a pre-requisite for adding aggregation by process name and by thread name to the Universal Profiling flamegraph view. It adds three artificial node types to the flamegraph including color codes. As a side-effect, the root node now has its own color code. Previously, it (accidentally) used the color code of "unknown" type frames. The PR is backwards compatible, so it doesn't change anything in the UI when connecting with current Elasticsearch. As soon as [the PR for ES](elastic/elasticsearch#119115) is merged, the new aggregations show up.

danielmitterdorfer

We had an offline conversation about the open questions, which are all resolved now. LGTM

…, root) (elastic#204977) ## Summary This PR is a pre-requisite for adding aggregation by process name and by thread name to the Universal Profiling flamegraph view. It adds three artificial node types to the flamegraph including color codes. As a side-effect, the root node now has its own color code. Previously, it (accidentally) used the color code of "unknown" type frames. The PR is backwards compatible, so it doesn't change anything in the UI when connecting with current Elasticsearch. As soon as [the PR for ES](elastic/elasticsearch#119115) is merged, the new aggregations show up.

rockdaboot added >enhancement :UniversalProfiling/Application Elastic Universal Profiling REST APIs and infrastructure Team:obs-knowledge Meta label for Observability Knowledge team v9.0.0 labels Dec 19, 2024

rockdaboot self-assigned this Dec 19, 2024

elasticsearchmachine added Team:obs-ux-infra_services Observability Infrastructure & Services User Experience Team external-contributor Pull request authored by a developer outside the Elasticsearch team labels Dec 19, 2024

rockdaboot mentioned this pull request Dec 19, 2024

[Profiling] Add colors for new frame types (process name, thread name, root) elastic/kibana#204977

Merged

rockdaboot removed the external-contributor Pull request authored by a developer outside the Elasticsearch team label Dec 19, 2024

rockdaboot requested review from christos68k and danielmitterdorfer December 19, 2024 16:50

rockdaboot force-pushed the flamegraph-executable-name branch 2 times, most recently from f9ecda3 to df54aca Compare January 2, 2025 17:29

christos68k reviewed Jan 4, 2025

View reviewed changes

...ore/template-resources/src/main/resources/profiling/component-template/profiling-events.json Show resolved Hide resolved

rockdaboot force-pushed the flamegraph-executable-name branch from df81709 to 52d909b Compare January 4, 2025 12:55

rockdaboot and others added 13 commits January 6, 2025 08:34

Add field process.executable.name to profiling-events

4d35b22

Amend query to aggregate events by executable name

a4aea04

Send flamegraph row with grouping by executable name

adae211

Flamegraph sub-aggregation by thread name

29fa515

Rework internal data model

ea63b21

Cleanups

823579c

Fix building tests

72254fb

Fix GetStackTracesResponseTests

1d2b984

Fix unit tests

535d30d

Fix remaining unit tests

b651317

[CI] Auto commit changes from spotless

398fc60

Fix flamegraph yaml tests

0a33c7e

Fix yaml REST tests

8046c83

Increase INDEX_TEMPLATE_VERSION for profiling.executable.name

9cf66c8

rockdaboot force-pushed the flamegraph-executable-name branch from 36f1696 to 9cf66c8 Compare January 6, 2025 07:38

rockdaboot requested a review from christos68k January 6, 2025 07:39

emma-raffenne removed the Team:obs-knowledge Meta label for Observability Knowledge team label Jan 6, 2025

Fix yamlRestCompatTest

cf4d652

Co-authored-by: Jake Landis <[email protected]>

christos68k approved these changes Jan 7, 2025

View reviewed changes

rockdaboot changed the title ~~[Profiling] Aggregate flamegraph by executable name and thread name~~ [Profiling] Aggregate flamegraph by process name and thread name Jan 8, 2025

Rename executable name to process name

e514874

rockdaboot added 2 commits January 8, 2025 16:08

Merge branch 'main' into flamegraph-executable-name

28c990c

Merge branch 'main' into flamegraph-executable-name

4fdd4d8

danielmitterdorfer reviewed Jan 10, 2025

View reviewed changes

rockdaboot and others added 7 commits January 10, 2025 17:19

Merge branch 'main' into flamegraph-executable-name

f3ca80c

Remove warnings meant for testing

fbb669b

Replace ChunkedToXContentHelper.wrapWithObject() with .object()

659a4fd

Fix comment in ProfilingIndexTemplateRegistry.java

f9ef6be

Simplify sorting of unique stacktrace and host IDs

62117b4

Merge branch 'main' into flamegraph-executable-name

57d3ad4

[CI] Auto commit changes from spotless

c118150

Merge branch 'main' into flamegraph-executable-name

7cd497c

danielmitterdorfer approved these changes Jan 14, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Profiling] Aggregate flamegraph by process name and thread name #119115

[Profiling] Aggregate flamegraph by process name and thread name #119115

rockdaboot commented Dec 19, 2024 •

edited

Loading

elasticsearchmachine commented Dec 19, 2024

elasticsearchmachine commented Dec 19, 2024

christos68k left a comment

danielmitterdorfer Jan 10, 2025

rockdaboot Jan 13, 2025 •

edited

Loading

danielmitterdorfer Jan 10, 2025

rockdaboot Jan 10, 2025 •

edited

Loading

danielmitterdorfer Jan 10, 2025

rockdaboot Jan 13, 2025 •

edited

Loading

danielmitterdorfer Jan 10, 2025

danielmitterdorfer left a comment

[Profiling] Aggregate flamegraph by process name and thread name #119115

Are you sure you want to change the base?

[Profiling] Aggregate flamegraph by process name and thread name #119115

Conversation

rockdaboot commented Dec 19, 2024 • edited Loading

Before

After

elasticsearchmachine commented Dec 19, 2024

elasticsearchmachine commented Dec 19, 2024

christos68k left a comment

Choose a reason for hiding this comment

danielmitterdorfer Jan 10, 2025

Choose a reason for hiding this comment

rockdaboot Jan 13, 2025 • edited Loading

Choose a reason for hiding this comment

danielmitterdorfer Jan 10, 2025

Choose a reason for hiding this comment

rockdaboot Jan 10, 2025 • edited Loading

Choose a reason for hiding this comment

danielmitterdorfer Jan 10, 2025

Choose a reason for hiding this comment

rockdaboot Jan 13, 2025 • edited Loading

Choose a reason for hiding this comment

danielmitterdorfer Jan 10, 2025

Choose a reason for hiding this comment

danielmitterdorfer left a comment

Choose a reason for hiding this comment

rockdaboot commented Dec 19, 2024 •

edited

Loading

rockdaboot Jan 13, 2025 •

edited

Loading

rockdaboot Jan 10, 2025 •

edited

Loading

rockdaboot Jan 13, 2025 •

edited

Loading