-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
feat: trace v4 integration #6226
Conversation
Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❌ Changes requested. Reviewed everything up to 1ba4d0f in 4 minutes and 56 seconds
More details
- Looked at
2242
lines of code in26
files - Skipped
0
files when reviewing. - Skipped posting
4
drafted comments based on config settings.
1. pkg/query-service/main.go:39
- Draft comment:
Avoid using inline styles in React components. This rule does not apply here as this is not a React component. - Reason this comment was not posted:
Confidence changes required:10%
The code uses inline styles in theinitZapLog
function, which is against the rule of avoiding inline styles in React components. However, this is not a React component, so this rule does not apply here. No other issues are found in this file.
2. pkg/query-service/main.go:51
- Draft comment:
Avoid using inline styles in React components. This rule does not apply here as this is not a React component. - Reason this comment was not posted:
Confidence changes required:10%
The code uses inline styles in theinitZapLog
function, which is against the rule of avoiding inline styles in React components. However, this is not a React component, so this rule does not apply here. No other issues are found in this file.
3. pkg/query-service/main.go:89
- Draft comment:
Avoid using inline styles in React components. This rule does not apply here as this is not a React component. - Reason this comment was not posted:
Confidence changes required:10%
The code uses inline styles in theinitZapLog
function, which is against the rule of avoiding inline styles in React components. However, this is not a React component, so this rule does not apply here. No other issues are found in this file.
4. pkg/query-service/main.go:94
- Draft comment:
Avoid using inline styles in React components. This rule does not apply here as this is not a React component. - Reason this comment was not posted:
Confidence changes required:10%
The code uses inline styles in theinitZapLog
function, which is against the rule of avoiding inline styles in React components. However, this is not a React component, so this rule does not apply here. No other issues are found in this file.
Workflow ID: wflow_nJVGVurFSah6sxTB
Want Ellipsis to fix these issues? Tag @ellipsis-dev
in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
I won't be able to review +900 -250 changes in one PR. Please break it down. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Looks good to me! Incremental review on d87c1f0 in 41 seconds
More details
- Looked at
87
lines of code in3
files - Skipped
0
files when reviewing. - Skipped posting
2
drafted comments based on config settings.
1. pkg/query-service/model/trace.go:5
- Draft comment:
TheSpanItemV2
struct is a duplicate of the removedSearchSpanResponseItemV2
struct fromresponse.go
. Ensure consistency between these definitions. - Reason this comment was not posted:
Confidence changes required:50%
TheSpanItemV2
struct intrace.go
is essentially a duplicate of theSearchSpanResponseItemV2
struct that was removed fromresponse.go
. This change is consistent with the PR description, which mentions moving models to support the new schema. However, theSpanItemV2
struct should be checked for any discrepancies with the originalSearchSpanResponseItemV2
to ensure consistency.
2. pkg/query-service/app/clickhouseReader/reader.go:1137
- Draft comment:
UseSpanItemV2
consistently across the codebase to avoid confusion and ensure proper organization. - 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 suggesting something that is already being addressed by the code change in the diff. There is no additional action required beyond what is already being done. The comment does not provide new information or point out an unresolved issue.
I might be missing the broader context of the codebase whereSpanItemV2
might not be used consistently elsewhere, but the comment does not specify any such instances.
The comment should specify if there are other instances whereSpanItemV2
is not used consistently. Without such information, the comment does not add value.
The comment does not point out any additional action required beyond what is already being done in the diff. It should be removed as it does not provide new information or point out an unresolved issue.
Workflow ID: wflow_EWJ19z6qt2MX9dMJ
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Looks good to me! Reviewed everything up to c31e9cd in 1 minute and 22 seconds
More details
- Looked at
1097
lines of code in21
files - Skipped
0
files when reviewing. - Skipped posting
3
drafted comments based on config settings.
1. ee/query-service/app/api/traces.go:30
- Draft comment:
Remove commented-out code if it's no longer needed to keep the codebase clean and maintainable. - Reason this comment was not posted:
Confidence changes required:50%
The PR introduces a new flaguse-trace-new-schema
to toggle between old and new trace schemas. This flag is consistently passed through various components, ensuring that the system can switch between schemas based on this flag. However, there is a potential issue with the commented-out code insearchTraces
function intraces.go
. The commented code should be removed if it's no longer needed, as it can lead to confusion and clutter the codebase.
2. pkg/query-service/app/clickhouseReader/reader.go:229
- Draft comment:
Remove commented-out code if it's no longer needed to keep the codebase clean and maintainable. - Reason this comment was not posted:
Confidence changes required:50%
The PR introduces a new flaguse-trace-new-schema
and updates various components to use this flag. However, there is a potential issue with the commented-out code insearchTraces
function intraces.go
. The commented code should be removed if it's no longer needed, as it can lead to confusion and clutter the codebase.
3. pkg/query-service/app/clickhouseReader/reader.go:189
- Draft comment:
Remove commented-out code if it's no longer needed to keep the codebase clean and maintainable. - Reason this comment was not posted:
Confidence changes required:50%
The PR introduces a new flaguse-trace-new-schema
and updates various components to use this flag. However, there is a potential issue with the commented-out code insearchTraces
function intraces.go
. The commented code should be removed if it's no longer needed, as it can lead to confusion and clutter the codebase.
Workflow ID: wflow_TzEz40CuqtyEk22w
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is not much to review here. Please throughly test.
Have changes for
use-trace-new-schema
, which when set true uses all the updated function for the new schemaAlso please be more cautious and strict while reviewing this PR , thanks
FOR #5713
Important
Introduces
use-trace-new-schema
flag to enable new trace schema, updating APIs, models, and constants to support it.use-trace-new-schema
flag inapi.go
,server.go
, andmain.go
to toggle new trace schema.GetServicesList
,SearchTracesV2
,GetTraceAggregateAttributesV2
,GetTraceAttributeKeysV2
,GetTraceAttributeValuesV2
,GetSpanAttributeKeysV2
to use new schema.GetTotalSpans
,GetSpansInLastHeartBeatInterval
,GetTagsInfoInLastHeartBeatInterval
for new schema.SearchSpanResponseItemV2
andTraceSummary
toresponse.go
.NewStaticFieldsTraces
andDeprecatedStaticFieldsTraces
inconstants.go
.ThresholdRule
inthreshold_rule.go
to support new schema.threshold_rule_test.go
for new schema.logs.go
andlogs_test.go
for timestamp range handling.This description was created by for c31e9cd. It will automatically update as commits are pushed.