fix: logs window based pagination to pageSize offset instead of using…

[nityanandagohain]

Fixes #6829

changes:-

• update window based pagination logic to support limit, pageSize, offset instead of id based filtering for logs.
• the logic is now similar to traces where offset is done locally.
• the tests are same as traces, but what changed is used of pageSize instead of limit as limit is the max amount of data to be paginated using offset and pageSize for logs.
• There is no max value of offset right now.
• changes in querier/querier v2 is same .

@srikanthccv requesting your review here as you have context on this part.

---

Important

Updates logs pagination to use pageSize and offset, aligning with traces logic, and updates tests accordingly.

• Behavior:
  • Updates window-based pagination logic in querier.go and querier/v2/querier.go to use pageSize and offset for logs, similar to traces.
  • Removes id-based filtering for logs.
  • No max value for offset currently.
• Tests:
  • Updates tests in querier_test.go and querier/v2/querier_test.go to reflect changes in pagination logic.
  • Adds tests for ascending and descending order queries with pageSize and offset.
• Misc:
  • Adds id DESC to default order by clause in query_builder.go for logs.

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

[ellipsis-dev]

👍 Looks good to me! Reviewed everything up to d0a130f in 1 minute and 11 seconds

More details

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

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

• Draft comment:
  The comment mentions a TODO for defining a limit for logs pagination. Consider addressing this or creating a follow-up task to ensure it's not forgotten.
• Reason this comment was not posted:
  Confidence changes required: 50%
  The PR changes the logic for pagination in logs queries to use pageSize and offset instead of id-based filtering. This change is reflected in the PrepareLogsQuery function and the runWindowBasedListQuery method. The tests have been updated to reflect these changes, ensuring that the new pagination logic is correctly implemented and tested. The PR description and issue indicate that the goal is to remove id from filters and use offset-based pagination, which is achieved in this PR.

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

• Draft comment:
  The comment mentions a TODO for defining a limit for logs pagination. Consider addressing this or creating a follow-up task to ensure it's not forgotten.
• Reason this comment was not posted:
  Confidence changes required: 50%
  The PR changes the logic for pagination in logs queries to use pageSize and offset instead of id-based filtering. This change is reflected in the PrepareLogsQuery function and the runWindowBasedListQuery method. The tests have been updated to reflect these changes, ensuring that the new pagination logic is correctly implemented and tested. The PR description and issue indicate that the goal is to remove id from filters and use offset-based pagination, which is achieved in this PR.

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

• 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 multiple files in this PR.
• Reason this comment was not posted:
  Comment was not on a valid diff hunk.

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

• 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 multiple files in this PR.
• Reason this comment was not posted:
  Marked as duplicate.

Workflow ID: wflow_wnWrPRGO2OJr3e7o

---

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

[srikanthccv]

Please share the plan. I see this is still not merged SigNoz/signoz-otel-collector#489

[nityanandagohain]

okay so here is the release plan.

• We will release the frontend and query service changes first. This is because this logic will work with existing id as well.
• Once done then we will release the collector changes in the next release.

[srikanthccv]

1. Why do we use different pagination mechanisms for traces and logs?
2. When there is no explicit order you should change this

   signoz/pkg/query-service/app/logs/v4/query_builder.go

   Line 275 in c56345e

   orderByArray = append(orderByArray, constants.TIMESTAMP+" DESC")

   to include id?

[nityanandagohain]

1. Why do we use different pagination mechanisms for traces and logs?

• So traces was built just with limit, offset. In logs we use limit, offset and pageSize. So for pagination limit, pageSize is used and limit is used to define the max limit pagination can be done. The traces UI was already using the limit, offset. But logs we decided to use limit, PageSize since we were using id pagination at that time.

2. Yeah correct, I will update it to have id as well for logs.

[srikanthccv]

				v.OrderBy[0].ColumnName == "timestamp" &&
				v.OrderBy[0].Order == "desc" &&
				v.OrderBy[1].ColumnName == "id" &&
				v.OrderBy[1].Order == "desc" {

Is there a reason why only desc uses the window search now? Why can't the same be done for asc?

[nityanandagohain]

So in this PR, wanted to focus on fixing the pagination just for the default page. So kept support for pagination asc aside.

[srikanthccv]

Ok, if there is no technical limitation to make the asc follow the same window-based search, it seemed like a small change so I brought it up.

[nityanandagohain]

Sure, let me make the change. Make sense to do it now as might be left out.

[nityanandagohain]

@srikanthccv I have updated the code to support order by asc both for logs and traces. added tests as well, please have a look

also the previous mock results were, incorrect i.e the mock use to return data in asc order even though the query was order by desc. Have fixed that as well.

[ellipsis-dev]

👍 Looks good to me! Incremental review on 0040169 in 1 minute and 12 seconds

More details

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

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

• Draft comment:
  The logic for updating limitWithOffset is inconsistent between logs and traces. Ensure that limitWithOffset is correctly updated for both logs and traces to avoid incorrect pagination behavior.
• Reason this comment was not posted:
  Comment did not seem useful.

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

• Draft comment:
  The logic for updating limitWithOffset is inconsistent between logs and traces. Ensure that limitWithOffset is correctly updated for both logs and traces to avoid incorrect pagination behavior.
• Reason this comment was not posted:
  Marked as duplicate.

Workflow ID: wflow_mareoteHuPTh8Ms3

---

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

[srikanthccv]

LGTM, let me know if you need approval now or later when it is verified with frontend integration.