feat: support for window based pagination in new trace v4 #6440

nityanandagohain · 2024-11-14T10:40:28Z

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 71b2e58. 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.

pkg/query-service/app/querier/querier.go

pkg/query-service/app/querier/v2/querier.go

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.

pkg/query-service/app/querier/v2/querier.go

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.

srikanthccv · 2024-11-15T04:30:30Z

Here is a case (i have been trying to come up with countercases against the idea of resetting offset to 0)

offset=100, limit =60

[t1-t2] = 50
[t2-t3] = 40
[t3-t4] = 30
[t4-t5] = 10
[t5-t6] = 20

srikanthccv · 2024-11-15T04:50:21Z

[t1, t2]: 30 traces
[t2, t3]: 40 traces
[t3, t4]: 50 traces

Offset: 50, Limit: 100

nityanandagohain · 2024-11-15T06:00:09Z

Yeah makes sense. To solve this I have a solution where if there is an offset and the result is empty then we do a temporary query with the same filters where limit = offset, and offset=0 to get the count of data that is skipped by offset.

and then for the next run, we can say offset = offset - count of data skipped from prev query .

@srikanthccv let me know if this sounds good

srikanthccv · 2024-11-15T06:14:33Z

can we do something to reduce the number of roundtrips to db?

nityanandagohain · 2024-11-15T06:17:19Z

One way I can think of is to not set offset at all, i.e we get the data and skip manually. Which should be fine unless the offset is very huge.

srikanthccv · 2024-11-15T06:58:08Z

I am OK with the product restriction that says For any list query, you can only consume X spans at the maximum; beyond that, you need to refine the search. IMO, 10k spans at maximum (all pages considered) in a list query is enough for all practical purposes (except when all 10k belong to the same trace, then it's not very useful because all of them open the same trace detail). Please bring it up in #product-pod and see what people think.

Alternatively, to handle large offsets, one option could be to perform a binary search on the tsRanges buckets. This would help identify which set of buckets effectively negate the offset, allowing you to start from the appropriate bucket with the offset reset to zero.

nityanandagohain · 2024-11-15T07:15:21Z

@srikanthccv you mean that when paginating we go a max of offset 10k and then stop right ?

and you are suggesting this implementation ?

One way I can think of is to not set offset at all, i.e we get the data and skip manually. Which should be fine unless the offset is very huge.

srikanthccv · 2024-11-15T07:59:10Z

Yes, assuming everyone is fine with such limitation.

ellipsis-dev

❌ Changes requested. Incremental review on d3f7c99 in 45 seconds

More details

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

1. pkg/query-service/app/querier/v2/querier.go:350

Draft comment:
Typo in variable name errQuriesByName. Consider renaming to errQueriesByName for consistency and clarity. This issue is also present in querier.go.
Reason this comment was not posted:
Marked as duplicate.

Workflow ID: wflow_kmzRauZn9pWXv105

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.

pkg/query-service/app/querier/querier.go

ellipsis-dev

👍 Looks good to me! Incremental review on 6bd56b0 in 43 seconds

More details

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

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

Draft comment:
Typo in variable name. Consider renaming errQuriesByName to errQueriesByName for consistency and clarity.
Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The comment is about a typo that has already been corrected in the diff. Since the change has been made, the comment is no longer necessary. The comment does not provide any additional value now that the issue is resolved.
I might be missing the context of whether the comment was meant to address a different part of the code that wasn't changed. However, based on the diff, it seems the issue is resolved.
The diff clearly shows that the typo has been corrected, so the comment is outdated and unnecessary.
The comment should be deleted because the typo it addresses has already been corrected in the diff.

2. pkg/query-service/app/querier/querier.go:401

Draft comment:
Typo in variable name. Consider renaming errQuriesByName to errQueriesByName for consistency and clarity.
Reason this comment was not posted:
Marked as duplicate.

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

Draft comment:
Typo in variable name. Consider renaming errQuriesByName to errQueriesByName for consistency and clarity.
Reason this comment was not posted:
Marked as duplicate.

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

Draft comment:
Typo in variable name. Consider renaming errQuriesByName to errQueriesByName for consistency and clarity.
Reason this comment was not posted:
Marked as duplicate.

5. pkg/query-service/app/querier/v2/querier.go:401

Draft comment:
Typo in variable name. Consider renaming errQuriesByName to errQueriesByName for consistency and clarity.
Reason this comment was not posted:
Marked as duplicate.

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

Draft comment:
Typo in variable name. Consider renaming errQuriesByName to errQueriesByName for consistency and clarity.
Reason this comment was not posted:
Marked as duplicate.

Workflow ID: wflow_I1QDs2SoP8wlTjvk

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

srikanthccv · 2024-11-15T14:40:15Z

Are you going to implement the 10k restriction separately or do you plan to update the same PR?

ellipsis-dev

👍 Looks good to me! Incremental review on 71b2e58 in 55 seconds

More details

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

1. pkg/query-service/constants/constants.go:594

Draft comment:
The constant TRACE_V4_MAX_PAGINATION_LIMIT is defined here but not used in this file. Ensure it's used where necessary in other parts of the codebase.
Reason this comment was not posted:
Confidence changes required: 10%
The constant TRACE_V4_MAX_PAGINATION_LIMIT is defined but not used in this file. It is used in other files to limit pagination, which is correct.

2. pkg/query-service/app/querier/querier.go:388

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 is applicable in other files as well.
Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The comment is not relevant to the changes made in the diff. The diff is about Go code, specifically related to query handling, and does not involve any React components or file structure changes. Therefore, the comment should be removed.
I might be missing some context about the overall project structure, but based on the provided diff, the comment is not relevant.
The diff is clearly about Go code, and there is no indication of any React component or file structure changes. The comment is not applicable here.
The comment is not relevant to the changes made in the diff and should be deleted.

3. pkg/query-service/app/querier/v2/querier.go:388

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 is applicable in other files as well.
Reason this comment was not posted:
Marked as duplicate.

Workflow ID: wflow_wdIgezVRfEIkdh1f

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

nityanandagohain · 2024-11-15T15:02:57Z

I have added it to this PR only @srikanthccv . Sorry for making this PR a pain to review.

feat: support for window based pagination in new trace v4

5bf01a7

github-actions bot added docs required enhancement New feature or request labels Nov 14, 2024

nityanandagohain marked this pull request as ready for review November 14, 2024 10:53

nityanandagohain requested review from srikanthccv, shivanshuraj1333 and eKuG November 14, 2024 10:53

ellipsis-dev bot reviewed Nov 14, 2024

View reviewed changes

pkg/query-service/app/querier/querier.go Show resolved Hide resolved

srikanthccv reviewed Nov 14, 2024

View reviewed changes

pkg/query-service/app/querier/v2/querier.go Outdated Show resolved Hide resolved

srikanthccv reviewed Nov 14, 2024

View reviewed changes

pkg/query-service/app/querier/v2/querier.go Outdated Show resolved Hide resolved

fix: update pagination logic

5077726

ellipsis-dev bot reviewed Nov 14, 2024

View reviewed changes

fix: update comment

373397c

ellipsis-dev bot reviewed Nov 14, 2024

View reviewed changes

srikanthccv reviewed Nov 14, 2024

View reviewed changes

pkg/query-service/app/querier/v2/querier.go Outdated Show resolved Hide resolved

fix: substract correct length

7366913

ellipsis-dev bot reviewed Nov 14, 2024

View reviewed changes

fix: revert changes

b8a803f

ellipsis-dev bot reviewed Nov 14, 2024

View reviewed changes

fix: add tests for querier

dee206c

ellipsis-dev bot reviewed Nov 14, 2024

View reviewed changes

fix: rename matcher

62c6c9b

ellipsis-dev bot reviewed Nov 14, 2024

View reviewed changes

fix: handle offset inmemory for list queries in traces

d3f7c99

ellipsis-dev bot reviewed Nov 15, 2024

View reviewed changes

pkg/query-service/app/querier/querier.go Outdated Show resolved Hide resolved

fix: correct var name

6bd56b0

ellipsis-dev bot reviewed Nov 15, 2024

View reviewed changes

nityanandagohain requested a review from srikanthccv November 15, 2024 11:11

fix: add max pagination limit for traces

71b2e58

ellipsis-dev bot reviewed Nov 15, 2024

View reviewed changes

srikanthccv approved these changes Nov 15, 2024

View reviewed changes

Merge branch 'develop' into feat/trace-v4-pagination

288e637

nityanandagohain added docs not required and removed docs required labels Nov 15, 2024

nityanandagohain merged commit 77c5f17 into develop Nov 15, 2024
17 checks passed

nityanandagohain deleted the feat/trace-v4-pagination branch November 15, 2024 16:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: support for window based pagination in new trace v4 #6440

feat: support for window based pagination in new trace v4 #6440

nityanandagohain commented Nov 14, 2024 •

edited by ellipsis-dev bot

Loading

ellipsis-dev bot left a comment

ellipsis-dev bot left a comment

ellipsis-dev bot left a comment

ellipsis-dev bot left a comment

ellipsis-dev bot left a comment

ellipsis-dev bot left a comment

ellipsis-dev bot left a comment

srikanthccv commented Nov 15, 2024 •

edited

Loading

srikanthccv commented Nov 15, 2024

nityanandagohain commented Nov 15, 2024 •

edited

Loading

srikanthccv commented Nov 15, 2024

nityanandagohain commented Nov 15, 2024

srikanthccv commented Nov 15, 2024

nityanandagohain commented Nov 15, 2024 •

edited

Loading

srikanthccv commented Nov 15, 2024

ellipsis-dev bot left a comment

ellipsis-dev bot left a comment

srikanthccv commented Nov 15, 2024

ellipsis-dev bot left a comment

nityanandagohain commented Nov 15, 2024

feat: support for window based pagination in new trace v4 #6440

feat: support for window based pagination in new trace v4 #6440

Conversation

nityanandagohain commented Nov 14, 2024 • edited by ellipsis-dev bot Loading

ellipsis-dev bot left a comment

Choose a reason for hiding this comment

ellipsis-dev bot left a comment

Choose a reason for hiding this comment

ellipsis-dev bot left a comment

Choose a reason for hiding this comment

ellipsis-dev bot left a comment

Choose a reason for hiding this comment

ellipsis-dev bot left a comment

Choose a reason for hiding this comment

ellipsis-dev bot left a comment

Choose a reason for hiding this comment

ellipsis-dev bot left a comment

Choose a reason for hiding this comment

srikanthccv commented Nov 15, 2024 • edited Loading

srikanthccv commented Nov 15, 2024

nityanandagohain commented Nov 15, 2024 • edited Loading

srikanthccv commented Nov 15, 2024

nityanandagohain commented Nov 15, 2024

srikanthccv commented Nov 15, 2024

nityanandagohain commented Nov 15, 2024 • edited Loading

srikanthccv commented Nov 15, 2024

ellipsis-dev bot left a comment

Choose a reason for hiding this comment

ellipsis-dev bot left a comment

Choose a reason for hiding this comment

srikanthccv commented Nov 15, 2024

ellipsis-dev bot left a comment

Choose a reason for hiding this comment

nityanandagohain commented Nov 15, 2024

nityanandagohain commented Nov 14, 2024 •

edited by ellipsis-dev bot

Loading

srikanthccv commented Nov 15, 2024 •

edited

Loading

nityanandagohain commented Nov 15, 2024 •

edited

Loading

nityanandagohain commented Nov 15, 2024 •

edited

Loading