-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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 support for span scope #6910
Conversation
Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id> |
Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id> |
There was a problem hiding this 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 d9a69ef in 2 minutes and 49 seconds
More details
- Looked at
195
lines of code in3
files - Skipped
0
files when reviewing. - Skipped posting
5
drafted comments based on config settings.
1. frontend/src/container/QueryBuilder/filters/QueryBuilderSearch/SpanScopeSelector.tsx:36
- Draft comment:
Usinguuid().slice(0, 8)
reduces the uniqueness of the ID. Consider using the full UUID to avoid potential collisions. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
Even with 8 characters of hex (32 bits), the chance of collision is extremely low for the small number of filter items that would exist simultaneously. The full UUID would be overkill for this use case. The tradeoff of shorter IDs for readability and debugging seems reasonable here.
I could be underestimating the number of filter items that might exist. There could be edge cases where many filters are created rapidly.
Even with hundreds of filters, the collision probability with 8 hex chars would be negligible. The code suggests this is used for UI filtering where having huge numbers of filters is unlikely.
The comment should be deleted as it raises a theoretical concern that is not a practical issue in this context. The current implementation is a reasonable tradeoff between uniqueness and usability.
2. frontend/src/container/QueryBuilder/filters/QueryBuilderSearch/SpanScopeSelector.tsx:85
- Draft comment:
UsingcloneDeep
can be expensive. Consider if a shallow copy would suffice or explore more efficient ways to update the query. - Reason this comment was not posted:
Confidence changes required:50%
ThecloneDeep
function from lodash is used to create a deep copy of thecurrentQuery
. This can be expensive in terms of performance, especially ifcurrentQuery
is large. Consider if a shallow copy would suffice or if there's a more efficient way to update the query.
3. frontend/src/container/QueryBuilder/filters/QueryBuilderSearch/SpanScopeSelector.tsx:75
- Draft comment:
EnsurecurrentQuery
inuseEffect
dependencies is not causing unnecessary re-renders. Consider optimizing dependencies. - Reason this comment was not posted:
Confidence changes required:50%
TheuseEffect
hook inSpanScopeSelector
depends oncurrentQuery
andqueryName
. IfcurrentQuery
is a complex object, it might cause unnecessary re-renders. Consider optimizing the dependency array or using a more specific dependency.
4. frontend/src/container/QueryBuilder/filters/QueryBuilderSearch/SpanScopeSelector.tsx:47
- Draft comment:
EnsureSELECT_OPTIONS
doesn't need to be dynamic. It's good for performance as it's defined outside the component. - Reason this comment was not posted:
Confidence changes required:20%
TheSELECT_OPTIONS
array is defined outside the component, which is good for performance as it avoids re-creation on each render. However, ensure that it doesn't need to be dynamic based on props or state.
5. frontend/src/container/QueryBuilder/filters/QueryBuilderSearch/index.tsx:56
- Draft comment:
Avoid usingindex.tsx
for component files. It makes it difficult to debug and find components using global search tools. - Reason this comment was not posted:
Comment was not on a valid diff hunk.
Workflow ID: wflow_VXHSrrYSvS7idYzn
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.
frontend/src/container/QueryBuilder/filters/QueryBuilderSearch/SpanScopeSelector.tsx
Outdated
Show resolved
Hide resolved
Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id> |
There was a problem hiding this 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 fa886bb in 28 seconds
More details
- Looked at
24
lines of code in2
files - Skipped
0
files when reviewing. - Skipped posting
2
drafted comments based on config settings.
1. frontend/src/container/QueryBuilder/filters/QueryBuilderSearch/SpanScopeSelector.tsx:122
- Draft comment:
Ensure that the.span-scope-selector
class does not conflict with other styles and is applied consistently across the application. - Reason this comment was not posted:
Confidence changes required:50%
The change from inline style to a CSS class is a good practice for maintainability and consistency. However, the CSS class.span-scope-selector
should be checked for any potential conflicts or unintended styling effects.
2. frontend/src/container/QueryBuilder/filters/QueryBuilderSearch/SpanScopeSelector.tsx:121
- Draft comment:
Avoid using inline styles in React components. Use CSS classes or styled components instead. - Reason this comment was not posted:
Comment looked like it was already resolved.
Workflow ID: wflow_hu8JvXW8lzphQrOV
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Functionality has been reviewed. It looks good.
@ankitnayan have a look.
fa886bb
to
f6a9093
Compare
Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id> |
Summary
Related Issues / PR's
Close https://github.com/SigNoz/engineering-pod/issues/2156
Screenshots
2025-01-23.16-45-24.mov
Affected Areas and Manually Tested Areas
Important
Adds
SpanScopeSelector
toQueryBuilderSearch
for span scope filtering on Traces Explorer page.SpanScopeSelector
component to allow selection of span scope (all, root, entrypoint spans).SpanScopeSelector
intoQueryBuilderSearch
only on Traces Explorer page..query-builder-search-container
and.span-scope-selector
styles inQueryBuilderSearch.styles.scss
.SpanScopeSelector.tsx
component for span scope selection.index.tsx
to includeSpanScopeSelector
and handle its state and effects.This description was created by for fa886bb. It will automatically update as commits are pushed.