feat: support for window based pagination in new trace v4

[nityanandagohain]

FOR #5713

Adds support for window based pagination for traces.

---

Important

Adds window-based pagination for trace queries with utility updates and a new schema flag in querier and querier/v2.

• Behavior:
  • Adds window-based pagination for trace queries in querier.go and querier/v2.go.
  • Introduces runWindowBasedListQuery() to handle pagination logic.
  • Updates runBuilderListQueries() to support trace data sources with pagination.
• Utils:
  • Renames GetLogsListTsRanges() to GetListTsRanges().
  • Updates TestLogsListTsRange() to TestListTsRange().
• Flags:
  • Adds UseTraceNewSchema flag in querier and querier/v2 to toggle new trace schema support.

^{This description was created by for 62c6c9b. It will automatically update as commits are pushed.}

[ellipsis-dev]

❌ Changes requested. Reviewed everything up to 5bf01a7 in 49 seconds

More details

• Looked at 228 lines of code in 4 files
• Skipped 0 files when reviewing.
• Skipped posting 6 drafted comments based on config settings.

1. pkg/query-service/app/querier/querier.go:310

• Draft comment:
  The function name was changed from runLogsListQuery to runWindowBasedListQuery, but the comment above still refers to logs. Consider updating the comment for clarity.
• Reason this comment was not posted:
  Confidence changes required: 50%
  The function name runLogsListQuery was changed to runWindowBasedListQuery in multiple places, but the comment above it still refers to logs. This should be updated for clarity.

2. pkg/query-service/app/querier/v2/querier.go:310

• Draft comment:
  The function name was changed from runLogsListQuery to runWindowBasedListQuery, but the comment above still refers to logs. Consider updating the comment for clarity.
• Reason this comment was not posted:
  Confidence changes required: 50%
  The function name runLogsListQuery was changed to runWindowBasedListQuery in multiple places, but the comment above it still refers to logs. This should be updated for clarity.

3. pkg/query-service/utils/logs.go:12

• Draft comment:
  The function GetLogsListTsRanges was renamed to GetListTsRanges, but the type LogsListTsRange was not renamed. Consider renaming the type for consistency.
• Reason this comment was not posted:
  Confidence changes required: 50%
  The function GetLogsListTsRanges was renamed to GetListTsRanges, but the type LogsListTsRange was not renamed. For consistency, consider renaming the type as well.

4. pkg/query-service/utils/logs_test.go:10

• Draft comment:
  The test function was renamed to TestListTsRange, but the test case names still refer to logs. Consider updating the test case names for consistency.
• Reason this comment was not posted:
  Confidence changes required: 50%
  The test function TestLogsListTsRange was renamed to TestListTsRange, but the test case names still refer to logs. Consider updating the test case names for consistency.

5. pkg/query-service/app/querier/querier.go:312

• Draft comment:
  The function runWindowBasedListQuery uses LogsListTsRange from the utils package, which is consistent with the updated function name GetListTsRanges. Ensure this change is reflected throughout the codebase.
• Reason this comment was not posted:
  Confidence changes required: 20%
  The function runWindowBasedListQuery in both querier.go and v2/querier.go files is using the LogsListTsRange type from utils package. The function GetLogsListTsRanges was renamed to GetListTsRanges, and the test function was also updated accordingly. This change is consistent across the codebase.

6. pkg/query-service/app/querier/v2/querier.go:312

• Draft comment:
  The function runWindowBasedListQuery uses LogsListTsRange from the utils package, which is consistent with the updated function name GetListTsRanges. Ensure this change is reflected throughout the codebase.
• Reason this comment was not posted:
  Confidence changes required: 20%
  The function runWindowBasedListQuery in both querier.go and v2/querier.go files is using the LogsListTsRange type from utils package. The function GetLogsListTsRanges was renamed to GetListTsRanges, and the test function was also updated accordingly. This change is consistent across the codebase.

Workflow ID: wflow_khr4V0jU0vDE3wFV

---

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.

[ellipsis-dev]

👍 Looks good to me! Incremental review on 5077726 in 22 seconds

More details

• Looked at 147 lines of code in 2 files
• Skipped 0 files when reviewing.
• Skipped posting 4 drafted comments based on config settings.

1. pkg/query-service/app/querier/querier.go:318

• Draft comment:
  Typo in comment: "se we are considering only one query" should be "we are considering only one query".
• Reason this comment was not posted:
  Confidence changes required: 10%
  The code in both files has a typo in the comment on line 318. It says "se we are considering only one query" which should be "we are considering only one query".

2. pkg/query-service/app/querier/v2/querier.go:318

• Draft comment:
  Typo in comment: "se we are considering only one query" should be "we are considering only one query".
• Reason this comment was not posted:
  Confidence changes required: 10%
  The code in both files has a typo in the comment on line 318. It says "se we are considering only one query" which should be "we are considering only one query".

3. pkg/query-service/app/querier/querier.go:318

• Draft comment:
  Typo in comment: replace 'se' with 'we'.
• Reason this comment was not posted:
  Confidence changes required: 10%
  The runWindowBasedListQuery function in both querier.go and v2/querier.go files contain a typo in the comment on line 318. The word 'se' should be 'we'.

4. pkg/query-service/app/querier/v2/querier.go:318

• Draft comment:
  Typo in comment: replace 'se' with 'we'.
• Reason this comment was not posted:
  Confidence changes required: 10%
  The runWindowBasedListQuery function in both querier.go and v2/querier.go files contain a typo in the comment on line 318. The word 'se' should be 'we'.

Workflow ID: wflow_Mtb7UHGHKrSBcsj5

---

You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

[ellipsis-dev]

👍 Looks good to me! Incremental review on 373397c in 14 seconds

More details

• Looked at 45 lines of code in 2 files
• Skipped 0 files when reviewing.
• Skipped posting 0 drafted comments based on config settings.

Workflow ID: wflow_H7k9Ag1UC3fu7U6L

---

You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

[srikanthccv]

You are over-subtracting by using the len of data? It should be subtracting the data fetched in the last recent request, not the whole data? Example, [t1-t2]:20, [t2-t3]:10, [t3-t4]:0, [t4:t5]:0, [t5:t6]:10, with offset = 0, limit = 40. after first iteration, the limit = 20, second iteration, the limit = 20 - (30) = -10.

Please add tests (use the clickhouse mocker to return rows).

[srikanthccv]

disregard, I see limit is a global value that's not changed on each iteration. I would still encourage adding tests.

[nityanandagohain]

For testing I will have to mock for reader, since I have to mock the function reader.GetListResultV3 . Any reference on how do I add that ?

[nityanandagohain]

Have added testes, please have a look again @srikanthccv

[ellipsis-dev]

👍 Looks good to me! Incremental review on 7366913 in 25 seconds

More details

• Looked at 80 lines of code in 2 files
• Skipped 0 files when reviewing.
• Skipped posting 2 drafted comments based on config settings.

1. pkg/query-service/app/querier/querier.go:338

• Draft comment:
  The comment 'this will to run only once' is misleading. The loop iterates over the 'queries' map, so it can run multiple times. Consider removing or correcting this comment.
• 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 incorrect because the loop iterates over a map, which can have multiple entries. The comment suggests a misunderstanding of the loop's behavior. The comment should likely be removed or corrected to reflect the actual behavior of the loop.
  I might be missing some context about the 'queries' map, such as if it is guaranteed to have only one entry in this specific context. However, without explicit evidence in the provided code, the comment appears misleading.
  The code does not provide any evidence that the 'queries' map will always have only one entry. Therefore, the comment is misleading and should be removed or corrected.
  The comment is misleading because the loop can run multiple times depending on the number of entries in the 'queries' map. The comment should be removed or corrected.

2. pkg/query-service/app/querier/v2/querier.go:338

• Draft comment:
  The comment 'this will to run only once' is misleading. The loop iterates over the 'queries' map, so it can run multiple times. Consider removing or correcting this comment.
• Reason this comment was not posted:
  Marked as duplicate.

Workflow ID: wflow_o1gmiLANWjDX3BdI

---

You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

[ellipsis-dev]

👍 Looks good to me! Incremental review on b8a803f in 14 seconds

More details

• Looked at 27 lines of code in 2 files
• Skipped 0 files when reviewing.
• Skipped posting 1 drafted comments based on config settings.

1. pkg/query-service/app/querier/querier.go:385

• Draft comment:
  The change from limit - length to limit - uint64(len(data)) ensures that the limit is adjusted based on the actual data length, which is more accurate. This change is consistent in both querier.go and querier/v2.go.
• Reason this comment was not posted:
  Confidence changes required: 10%
  The change from limit - length to limit - uint64(len(data)) is consistent in both files. This change ensures that the limit is adjusted based on the actual data length, which is a more accurate representation of the remaining limit.

Workflow ID: wflow_mlLaMu2CChPnVinF

---

You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

[ellipsis-dev]

👍 Looks good to me! Incremental review on dee206c in 44 seconds

More details

• Looked at 600 lines of code in 2 files
• Skipped 0 files when reviewing.
• Skipped posting 1 drafted comments based on config settings.

1. pkg/query-service/app/querier/querier_test.go:1148

• Draft comment:
  The test function Test_querier_runWindowBasedListQuery is duplicated in both querier_test.go and v2/querier_test.go. Consider refactoring to avoid duplication.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable:
  The comment is speculative as it refers to another file not included in the diff. Without evidence of duplication in the current file, the comment does not provide actionable feedback. The comment does not align with the rules as it is speculative and not based on changes in the current diff.
  The comment might be valid if there is indeed a duplication in another file, but without seeing that file, it's impossible to confirm. The comment does not provide enough context or evidence to be actionable.
  Even if the comment is correct, it does not pertain to changes made in this diff and lacks evidence. Therefore, it should be removed based on the rules provided.
  Remove the comment as it is speculative and not based on changes in the current diff. It lacks evidence and actionable feedback.

Workflow ID: wflow_Zj5BZVKqUjLpytxW

---

You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

[ellipsis-dev]

👍 Looks good to me! Incremental review on 62c6c9b in 33 seconds

More details

• Looked at 53 lines of code in 2 files
• Skipped 0 files when reviewing.
• Skipped posting 3 drafted comments based on config settings.

1. pkg/query-service/app/querier/querier_test.go:1131

• Draft comment:
  The renaming of queryMatcherAny to regexMatcher is consistent across both files, which is good. However, the change is purely cosmetic and doesn't affect functionality. Ensure the new name accurately reflects the purpose of the struct, which is to match SQL queries using regex.
• Reason this comment was not posted:
  Confidence changes required: 20%
  The renaming of queryMatcherAny to regexMatcher is consistent across both files, which is good. However, the change is purely cosmetic and doesn't affect functionality. It's important to ensure that the new name accurately reflects the purpose of the struct, which is to match SQL queries using regex.

2. pkg/query-service/app/querier/v2/querier_test.go:1188

• Draft comment:
  The renaming of queryMatcherAny to regexMatcher is consistent across both files, which is good. However, the change is purely cosmetic and doesn't affect functionality. Ensure the new name accurately reflects the purpose of the struct, which is to match SQL queries using regex.
• Reason this comment was not posted:
  Confidence changes required: 20%
  The renaming of queryMatcherAny to regexMatcher is consistent across both files, which is good. However, the change is purely cosmetic and doesn't affect functionality. It's important to ensure that the new name accurately reflects the purpose of the struct, which is to match SQL queries using regex.

3. pkg/query-service/app/querier/querier_test.go:1134

• Draft comment:
  Avoid using the component/index.tsx file structure approach, as it makes it difficult to debug and find components using global search tools like VS Code. This applies to the regexMatcher type definition here and in querier/v2/querier_test.go.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable:
  The comment is likely incorrect because it refers to a React component file structure, which is not applicable to a Go test file. The comment does not seem to address any changes made in the diff.
  I might be missing some context about the project structure, but based on the file content, the comment seems irrelevant.
  The file content and the nature of the comment strongly suggest that the comment is misplaced and irrelevant.
  The comment should be deleted as it is not relevant to the changes made in the diff.

Workflow ID: wflow_2m0NuCHHYcSoquoP

---

You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.