Skip to content
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

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

nityanandagohain
Copy link
Member

@nityanandagohain nityanandagohain commented Jan 16, 2025

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

Update logs pagination to use pageSize and offset, aligning with traces logic, and add tests for validation.

  • Behavior:
    • Update logs pagination to use pageSize and offset instead of id based filtering in PrepareLogsQuery in query_builder.go.
    • Align logs pagination logic with traces, handling offset locally in runWindowBasedListQuery in querier.go and v2/querier.go.
    • No maximum offset value is enforced.
  • Tests:
    • Add tests for logs pagination in querier_test.go and v2/querier_test.go to validate pageSize and offset logic.
  • Misc:
    • Minor adjustments in runBuilderListQueries to accommodate new pagination logic.

This description was created by Ellipsis for d0a130f. It will automatically update as commits are pushed.

@github-actions github-actions bot added the bug Something isn't working label Jan 16, 2025
@nityanandagohain nityanandagohain marked this pull request as ready for review January 16, 2025 09:38
Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a 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 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
Copy link
Member

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

@nityanandagohain
Copy link
Member Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[LOGS][BE]: API changes for pagination i.e remove id from filters.
2 participants