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: add search span scope in the list view tab to add the scope at per Query level #6810

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

Conversation

eKuG
Copy link
Contributor

@eKuG eKuG commented Jan 13, 2025

Summary

Capability we want to introduce with these change:

  • Add the search span scope in the query level.
  • The scope should include but not limited to searching on all spans, entrypoint spans and root spans
  • The code should be extensible. We can update the code whenever there is new requirement without changing the user experience.

Related Issues / PR's

#3267

Screenshots

NA

Affected Areas and Manually Tested Areas

The changes to the query range api will be as followed:

spanSearchScope field will be added in the builderQueries object

The spanSearch scope will have a default value of all_spans but the possible values are entry_point_spans and root_spans

More information can be found at this document : Notion Document


Important

Adds spanSearchScope to BuilderQuery for query-level span search control, supporting all_spans, entry_point_spans, and root_spans.

  • Behavior:
    • Adds spanSearchScope field to BuilderQuery in v3.go to specify search scope at query level.
    • Supports all_spans, entry_point_spans, and root_spans as possible values for spanSearchScope.
    • Default value for spanSearchScope is all_spans.
  • Query Building:
    • Modifies buildTracesQuery() in query_builder.go to handle spanSearchScope.
    • Adds logic to filter spans based on spanSearchScope in query_builder.go.
  • Constants:
    • Adds SIGNOZ_TOP_LEVEL_OPERATIONS_TABLENAME to constants.go.
  • Testing:
    • Adds test cases in query_builder_test.go for spanSearchScope handling in buildTracesQuery().

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

@github-actions github-actions bot added docs required enhancement New feature or request labels Jan 13, 2025
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. Reviewed everything up to 0346e52 in 2 minutes and 1 seconds

More details
  • Looked at 77 lines of code in 3 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. pkg/query-service/model/v3/v3.go:34
  • Draft comment:
    The SpanSearchScope type and its constants are defined in both pkg/query-service/app/traces/v4/query_builder.go and pkg/query-service/model/v3/v3.go. Consider defining them in one place to avoid redundancy.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable:
    The comment suggests there may be code duplication, but I can't verify this since I don't have access to the other file. The rules state that if understanding requires more context or seeing other files, we should delete the comment. Additionally, this seems like a cross-file issue which the rules explicitly say to ignore.
    I could be wrong about this being a cross-file issue - maybe having duplicate type definitions is a serious enough problem that it should be flagged regardless. The comment could be helping prevent maintenance issues.
    While duplicate type definitions can be problematic, the rules explicitly state to ignore cross-file issues and to delete comments if we need more context from other files to verify them. We must follow these rules.
    Delete this comment since it requires seeing another file to verify and involves a cross-file issue, both of which are explicitly against the review rules.

Workflow ID: wflow_KGDrTd6ZZKUanQ1K


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.

@ankitnayan
Copy link
Collaborator

add tests?

@ankitnayan ankitnayan removed their request for review January 13, 2025 13:53
@eKuG
Copy link
Contributor Author

eKuG commented Jan 13, 2025

@ankitnayan tests are added

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 75b4d89 in 1 minute and 7 seconds

More details
  • Looked at 45 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/v4/query_builder_test.go:552
  • 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 entire file.
  • Reason this comment was not posted:
    Comment was not on a valid diff hunk.

Workflow ID: wflow_P6Y4fCrbPHmfsaer


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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs shipped enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants