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

feat: move resource qb to its own package and use common options #6238

Merged
merged 3 commits into from
Oct 22, 2024

Conversation

nityanandagohain
Copy link
Member

@nityanandagohain nityanandagohain commented Oct 22, 2024

Changes

  • move resource qb to its own package.
  • use a common options struct.

-- this is done

  • traces will use the same resource qb
  • traces will have a similar v4 qb which will be behind a flag.

part of #5713


Important

Refactor codebase by moving resource query builder to a separate package and standardizing options struct across query builders.

  • Refactoring:
    • Move resource_query_builder.go from logs/v4 to resource package.
    • Update imports in query_builder.go and query_builder_test.go to use the new resource package.
  • Options Struct:
    • Replace LogQBOptions and Options with QBOptions in query_builder.go, helper.go, and v2/helper.go.
    • Update function signatures and calls to use QBOptions.
  • Functionality:
    • Use common resource query builder in logs/v4/query_builder.go.
    • Implement similar query builder for traces in traces/v3/query_builder_test.go.
  • Testing:
    • Update tests in query_builder_test.go and resource_query_builder_test.go to reflect changes in package structure and options usage.

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

@github-actions github-actions bot added the enhancement New feature or request label Oct 22, 2024
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 3808837 in 37 seconds

More details
  • Looked at 611 lines of code in 10 files
  • Skipped 0 files when reviewing.
  • Skipped posting 2 drafted comments based on config settings.
1. pkg/query-service/app/logs/v3/query_builder.go:496
  • Draft comment:
    The change from LogQBOptions to QBOptions is consistent with the intent to use a common options struct. Ensure that all references to LogQBOptions are updated to QBOptions across the codebase.
  • Reason this comment was not posted:
    Confidence changes required: 10%
    The PR changes the struct from LogQBOptions to QBOptions across multiple files. This change is consistent and aligns with the intent to use a common options struct. The change is reflected in function signatures, variable declarations, and test cases. The change is straightforward and does not introduce any logical or performance issues. The tests have been updated accordingly, ensuring that the new struct is used consistently. The PR description indicates that this is part of a larger refactoring effort, which is consistent with the changes made.
2. pkg/query-service/app/resource/resource_query_builder.go:1
  • 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 the pkg/query-service/app/resource/resource_query_builder.go file.
  • Reason this comment was not posted:
    Comment was on unchanged code.

Workflow ID: wflow_0rNoYbkckZ1qsi5H


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

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.

❌ Changes requested. Incremental review on 1920e5d in 41 seconds

More details
  • Looked at 36 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/traces/v3/query_builder.go:238
  • Draft comment:
    The Options struct is redundant and should be removed since QBOptions is now used. This applies to the entire file.
  • Reason this comment was not posted:
    Marked as duplicate.

Workflow ID: wflow_EPhHbmnCuOyScw4o


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.

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! Incremental review on 826c280 in 29 seconds

More details
  • Looked at 16 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. pkg/query-service/app/traces/v3/query_builder.go:10
  • Draft comment:
    The Options struct is removed but not replaced with QBOptions as mentioned in the PR description. Ensure that QBOptions is used where necessary.
  • Reason this comment was not posted:
    Comment did not seem useful.

Workflow ID: wflow_UH0jUvqrwswUunTA


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

@nityanandagohain nityanandagohain merged commit 2b0da82 into develop Oct 22, 2024
14 checks passed
@nityanandagohain nityanandagohain deleted the fix/resource_qb branch October 22, 2024 11:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants