feat(trace-details): query service changes for trace details #6906
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id> |
1 similar comment
|
Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id> |
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
👍 Looks good to me! Reviewed everything up to 835d7a3 in 3 minutes and 20 seconds
More details
- Looked at
1369lines of code in16files - Skipped
0files when reviewing. - Skipped posting
4drafted comments based on config settings.
1. pkg/query-service/app/server.go:161
- Draft comment:
EnsurefluxIntervalForTraceDetailis consistently used across all instances. Currently, it is passed asnilhere, which might lead to unexpected behavior. - Reason this comment was not posted:
Marked as duplicate.
2. pkg/query-service/app/server.go:183
- Draft comment:
EnsurefluxIntervalForTraceDetailis consistently used across all instances. Currently, it is passed asnilhere, which might lead to unexpected behavior. - Reason this comment was not posted:
Marked as duplicate.
3. pkg/query-service/app/server.go:134
- Draft comment:
EnsurefluxIntervalForTraceDetailis consistently used across all instances. Currently, it is passed asnilhere, which might lead to unexpected behavior. - Reason this comment was not posted:
Comment did not seem useful.
4. pkg/query-service/interfaces/interface.go:44
- Draft comment:
Do not add non-ClickHouse related functions to the ClickHouseReader interface. Use the DAO in the telemetry instance to access such information instead. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The comment seems to be based on a misunderstanding. This is a general Reader interface, not a ClickHouseReader interface. The new functions are trace-related operations which fit with other trace operations already in the interface (like SearchTraces). The interface already contains many non-ClickHouse specific operations. The comment appears to be enforcing a rule that doesn't apply here.
I could be wrong about the broader context - maybe there's a reason these specific functions should be handled differently from other trace operations. There might be architectural decisions I'm not aware of.
Even with potential missing context, the evidence strongly suggests this is a general interface for reading various types of data, including traces. The new methods are consistent with existing patterns in the interface.
The comment should be deleted because it appears to be based on a misunderstanding of the interface's purpose and scope. The new methods are consistent with the interface's existing functionality.
Workflow ID: wflow_shWExKQEufwO4YOY
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 90476d7 in 1 minute and 32 seconds
More details
- Looked at
1313lines of code in10files - Skipped
0files when reviewing. - Skipped posting
9drafted comments based on config settings.
1. pkg/query-service/app/clickhouseReader/reader.go:160
- Draft comment:
The import alias for the cache package has been changed fromcacheV2tocache, but the variable namecacheV2is still used in some places. Ensure consistency in naming to avoid confusion. - Reason this comment was not posted:
Comment looked like it was already resolved.
2. pkg/query-service/app/clickhouseReader/reader.go:175
- Draft comment:
The import alias for the cache package has been changed fromcacheV2tocache, but the variable namecacheV2is still used in some places. Ensure consistency in naming to avoid confusion. - Reason this comment was not posted:
Marked as duplicate.
3. pkg/query-service/app/clickhouseReader/reader.go:186
- Draft comment:
The import alias for the cache package has been changed fromcacheV2tocache, but the variable namecacheV2is still used in some places. Ensure consistency in naming to avoid confusion. - Reason this comment was not posted:
Marked as duplicate.
4. pkg/query-service/app/clickhouseReader/reader.go:288
- Draft comment:
The import alias for the cache package has been changed fromcacheV2tocache, but the variable namecacheV2is still used in some places. Ensure consistency in naming to avoid confusion. - Reason this comment was not posted:
Marked as duplicate.
5. pkg/query-service/app/clickhouseReader/reader.go:1454
- Draft comment:
The import alias for the cache package has been changed fromcacheV2tocache, but the variable namecacheV2is still used in some places. Ensure consistency in naming to avoid confusion. - Reason this comment was not posted:
Marked as duplicate.
6. pkg/query-service/app/clickhouseReader/reader.go:1553
- Draft comment:
The import alias for the cache package has been changed fromcacheV2tocache, but the variable namecacheV2is still used in some places. Ensure consistency in naming to avoid confusion. - Reason this comment was not posted:
Marked as duplicate.
7. pkg/query-service/app/traces/tracedetail/flamegraph.go:1
- Draft comment:
Avoid using thecomponent/index.tsxfile structure approach, as it makes it difficult to debug and find components using global search tools like VS Code. - Reason this comment was not posted:
Comment was not on a valid diff hunk.
8. pkg/query-service/app/traces/tracedetail/waterfall.go:1
- Draft comment:
Avoid using thecomponent/index.tsxfile structure approach, as it makes it difficult to debug and find components using global search tools like VS Code. - Reason this comment was not posted:
Marked as duplicate.
9. pkg/query-service/model/cacheable.go:1
- Draft comment:
Avoid using thecomponent/index.tsxfile structure approach, as it makes it difficult to debug and find components using global search tools like VS Code. - Reason this comment was not posted:
Marked as duplicate.
Workflow ID: wflow_zzD5Kbzef6WEAE9V
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on a1d3d6f in 38 seconds
More details
- Looked at
166lines of code in5files - Skipped
0files when reviewing. - Skipped posting
3drafted comments based on config settings.
1. pkg/query-service/app/traces/tracedetail/flamegraph.go:91
- Draft comment:
The logic for adjustinglowerLimitandupperLimitis repeated inGetSelectedSpansForFlamegraphForRequestandGetSelectedSpansinwaterfall.go. Consider refactoring this into a helper function to improve maintainability. - Reason this comment was not posted:
Confidence changes required:50%
The functionGetSelectedSpansForFlamegraphForRequestinflamegraph.gohas redundant code for adjusting thelowerLimitandupperLimit. The same logic is repeated inGetSelectedSpansinwaterfall.go. This redundancy can be refactored into a helper function to improve code maintainability.
2. pkg/query-service/app/traces/tracedetail/waterfall.go:106
- Draft comment:
The parameterisPartOfPreOrderintraverseTracecould be renamed to something more descriptive to indicate its purpose more clearly, such asshouldIncludeInPreOrder. - Reason this comment was not posted:
Confidence changes required:20%
ThetraverseTracefunction inwaterfall.gohas a parameterisPartOfPreOrderthat is used to determine if a node should be added to thepreOrderTraversal. This logic is correct, but the nameisPartOfPreOrdercould be more descriptive to indicate its purpose more clearly.
3. pkg/query-service/app/traces/tracedetail/waterfall.go:106
- Draft comment:
Avoid using inline styles in React components. Instead, use external stylesheets, CSS classes, or styled components. - Reason this comment was not posted:
Confidence changes required:50%
The functiontraverseTraceinwaterfall.gohas been modified to include an additional parameterisPartOfPreOrder, which is used to determine whether to append the node to the pre-order traversal. This change is consistent with the logic of maintaining a pre-order traversal based on certain conditions.
Workflow ID: wflow_Av3v4ufTz2pUfriC
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
grandwizard28
requested changes
Jan 23, 2025
grandwizard28
requested changes
Jan 23, 2025
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on b33aa74 in 29 seconds
More details
- Looked at
340lines of code in3files - Skipped
0files when reviewing. - Skipped posting
3drafted comments based on config settings.
1. pkg/query-service/app/traces/tracedetail/waterfall.go:4
- Draft comment:
The use of theslicespackage for theContainsfunction is a good replacement for the customcontainsfunction. This improves code readability and maintainability. - Reason this comment was not posted:
Confidence changes required:10%
Theslicespackage is used for theContainsfunction, which is a good replacement for the customcontainsfunction. This change is appropriate and improves code readability.
2. pkg/query-service/app/traces/tracedetail/waterfall.go:81
- Draft comment:
Theslices.Containsfunction is used correctly in place of the customcontainsfunction. This change is appropriate and improves code readability. This is also applicable at line 130. - Reason this comment was not posted:
Confidence changes required:10%
Theslices.Containsfunction is used correctly in place of the customcontainsfunction. This change is appropriate and improves code readability.
3. pkg/query-service/app/traces/tracedetail/waterfall.go:78
- Draft comment:
Thecontainsfunction is defined but not used. Consider removing it to clean up the code. - Reason this comment was not posted:
Confidence changes required:50%
The functioncontainsis defined but not used anywhere in the code. It should be removed to clean up the code.
Workflow ID: wflow_l5aBhccFBv3WkfPC
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 52ad95e in 1 minute and 27 seconds
More details
- Looked at
44lines of code in1files - Skipped
0files when reviewing. - Skipped posting
4drafted comments based on config settings.
1. pkg/query-service/app/clickhouseReader/reader.go:1456
- Draft comment:
The function signature has changed, addingtraceDetailsQueryas a parameter. Ensure all invocations ofGetSpansForTraceare updated accordingly. - Reason this comment was not posted:
Confidence changes required:50%
The change in the function signature ofGetSpansForTracerequires updating all its invocations. The PR author has done this, but it's important to ensure that all invocations are updated correctly. The change involves adding a new parametertraceDetailsQuery.
2. pkg/query-service/app/clickhouseReader/reader.go:1469
- Draft comment:
Logging SQL queries can expose sensitive information. Ensure this complies with security and privacy policies. - Reason this comment was not posted:
Confidence changes required:50%
The SQL query string is being logged, which can be useful for debugging but may expose sensitive information. It's important to ensure that logging this information does not violate any security or privacy policies.
3. pkg/query-service/app/clickhouseReader/reader.go:1523
- Draft comment:
Ensure thatfmt.Sprintfusage for SQL query construction does not allow user input to affect the query structure, preventing SQL injection. - Reason this comment was not posted:
Confidence changes required:50%
The SQL query string is being constructed usingfmt.Sprintf, which is generally safe here because the table names are not user inputs. However, it's important to ensure that no user input can affect the query structure to prevent SQL injection.
4. pkg/query-service/app/clickhouseReader/reader.go:1456
- Draft comment:
Avoid adding non-ClickHouse related parameters liketraceDetailsQueryto theClickHouseReaderinterface. Use the DAO in the telemetry instance to access such information instead. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The comment appears to be incorrect. ThetraceDetailsQueryparameter is directly related to ClickHouse functionality as it contains SQL queries executed against ClickHouse. This is not a non-ClickHouse related parameter. The parameter is used to make the query reusable across different trace detail retrieval scenarios.
Could I be wrong about the parameter's purpose? Maybe there's a better architectural pattern for handling different query variations?
While there could be other patterns, passing the query as a parameter is a valid approach here since it's still directly related to ClickHouse functionality. The parameter name could be clearer but the functionality is appropriate.
The comment should be deleted as it incorrectly identifies traceDetailsQuery as a non-ClickHouse related parameter when it is actually a ClickHouse SQL query parameter.
Workflow ID: wflow_p5S9N3tt3nQuDGoC
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
grandwizard28
approved these changes
Jan 23, 2025
| } | ||
|
|
||
| if time.Since(time.UnixMilli(int64(cachedTraceData.EndTime))) < r.fluxIntervalForTraceDetail { | ||
| zap.L().Info("the trace end time falls under the flux interval, skipping getWaterfallSpansForTraceWithMetadata cache", zap.String("traceID", traceID)) |
| } | ||
|
|
||
| if err != nil { | ||
| zap.L().Info("cache miss for getWaterfallSpansForTraceWithMetadata", zap.String("traceID", traceID)) |
| } | ||
| totalSpans = uint64(len(searchScanResponses)) | ||
|
|
||
| processingBeforeCache := time.Now() |
There was a problem hiding this comment.
this part can be it's own function ?
|
|
||
| // traverse through the map and append each node to the children array of the parent node | ||
| // and add the missing spans | ||
| for _, spanNode := range spanIdToSpanNodeMap { |
There was a problem hiding this comment.
this part as well, with tests.
|
|
||
| processingBeforeCache := time.Now() | ||
| for _, item := range searchScanResponses { | ||
| ref := []model.OtelSpanRef{} |
There was a problem hiding this comment.
duplicate parts from GetWaterfallSpansForTraceWithMetadata
| return selectedSpanIndex | ||
| } | ||
|
|
||
| func getPathFromRootToSelectedSpanId(node *model.Span, selectedSpanId string, uncollapsedSpans []string, isSelectedSpanIDUnCollapsed bool) (bool, []string) { |
There was a problem hiding this comment.
I don't see any test cases for functions in these files.
| } | ||
|
|
||
| func (c *GetWaterfallSpansForTraceWithMetadataCache) MarshalBinary() (data []byte, err error) { | ||
| return json.Marshal(c) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Related Issues / PR's
contributes to - #6132
Screenshots
NA
Affected Areas and Manually Tested Areas