Skip to content

Support cross-thread trace profiling#111

Merged
wu-sheng merged 4 commits intoapache:masterfrom
mrproliu:trace-profiling-cross-thread
Mar 13, 2023
Merged

Support cross-thread trace profiling#111
wu-sheng merged 4 commits intoapache:masterfrom
mrproliu:trace-profiling-cross-thread

Conversation

@mrproliu
Copy link
Copy Markdown
Contributor

@mrproliu mrproliu commented Mar 9, 2023

Following apache/skywalking#10373
We need to let GraphQL support query/analysis with multiple segments.

@mrproliu mrproliu requested a review from wu-sheng March 9, 2023 07:58
profile.graphqls Outdated
# query profiled segment
getProfiledSegment(segmentId: String): ProfiledSegment
# query combined segments by ref
getProfiledSegments(segmentIds: [String!]!): [ProfiledSegments!]!
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What do you mean by ref? It is better query by Trace ID?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This means combining all the same segments under one process.
Query by segments because this query only focuses on the profiled segments, not all segments under one trace.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Are you going to verify the segment data somehow?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

No verification, just query the segments and combine them when the cross-thread segment matches with the parent segment.
Then, The UI can show the spans in the same process, and analyze the profiling results with the same process(segment id list).

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

And why do we need ProfiledSegments Array and this new object rather than only ProfiledSegment array only?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

No verification, just query the segments and combine them when the cross-thread segment matches with the parent segment. Then, The UI can show the spans in the same process, and analyze the profiling results with the same process(segment id list).

I think UI is hard to verify things like ref linking segments. We may only bind segments with trace ID belonging to one task ID. This should be enough.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Because they may become different traces/processes, this decide by the query.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

No verification, just query the segments and combine them when the cross-thread segment matches with the parent segment. Then, The UI can show the spans in the same process, and analyze the profiling results with the same process(segment id list).

I think UI is hard to verify things like ref linking segments. We may only bind segments with trace ID belonging to one task ID. This should be enough.

Do you mean to query all segments in the same trace? But there may have multiple segments that are not profiled, so they cannot be analyzed.

profile.graphqls Outdated
@@ -156,6 +163,10 @@ extend type Query {
getProfileTaskSegmentList(taskID: String): [BasicTrace!]!
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@mrproliu How about UI enhancing this way,

  1. Use this API, to group segmentIds by traceId and Instance ID(new Added). Could our backend these without agent update?
  2. getProfiledSegments(new APIs) to get ProfiledSegments.
  3. Build profiling.

The major differences with yours are

  1. getProfiledSegment should be deprecated, only a shell(special condition) to getProfiledSegments.
  2. No new ProfiledSegments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants