-
Notifications
You must be signed in to change notification settings - Fork 1.6k
feat: add auto refresh to logs explorer #8004
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
base: main
Are you sure you want to change the base?
Conversation
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.
Important
Looks good to me! π
Reviewed everything up to 7a61129 in 1 minute and 26 seconds. Click for details.
- Reviewed
119
lines of code in7
files - Skipped
0
files when reviewing. - Skipped posting
8
draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with π or π to teach Ellipsis.
1. frontend/src/components/Graph/index.tsx:77
- Draft comment:
New time format mapping added. Ensure consistency and reuse of date format constants if available. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
2. frontend/src/container/QueryBuilder/components/Query/Query.tsx:326
- Draft comment:
Good use of optional chaining to safely check query.aggregateAttribute.key. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
This comment is purely informative, as it praises the use of optional chaining without providing any actionable feedback or suggestions. According to the rules, purely informative comments should be removed.
3. frontend/src/container/QueryBuilder/filters/QueryBuilderSearchV2/SpanScopeSelector.tsx:63
- Draft comment:
Optional chaining added on filters (using filters?.some) ensures safe iteration when filters is undefined. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
This comment is purely informative, explaining the use of optional chaining. It does not provide a suggestion, ask for confirmation, or point out a potential issue.
4. frontend/src/container/Trace/Graph/config.ts:33
- Draft comment:
Added a null check for allDataPoints before iterating its keys; this prevents potential runtime errors. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
This comment is purely informative, explaining what was done in the code without suggesting any changes or improvements. It doesn't align with the rules for useful comments, which should provide suggestions or ask for clarifications on specific code intentions.
5. frontend/src/pages/LogsExplorer/__tests__/LogsExplorer.test.tsx:240
- Draft comment:
New test added to verify the presence of auto refresh buttons in the toolbar. Nice addition to ensure the UI behavior. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
The comment is asking the PR author to ensure UI behavior consistency, which violates the rule against asking for confirmation or verification of behavior. It doesn't provide a specific suggestion or point out a specific issue with the code.
6. frontend/src/pages/LogsExplorer/index.tsx:224
- Draft comment:
Auto refresh feature is enabled by passing the 'showAutoRefresh' prop to Toolbar. Confirm that the Toolbar component correctly handles auto refresh behavior as expected. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
This comment is asking the PR author to confirm the behavior of theToolbar
component, which violates the rule against asking for confirmation of intended behavior. It does not provide a specific suggestion or point out a clear issue.
7. frontend/src/pages/TracesExplorer/index.tsx:80
- Draft comment:
Optional chaining for groupBy length ensures stability by handling cases where groupBy might be undefined. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
This comment is purely informative, explaining the use of optional chaining forgroupBy
. It doesn't provide a suggestion, ask for confirmation, or point out a potential issue.
8. frontend/src/pages/LogsExplorer/index.tsx:1
- Draft comment:
The PR description mentions resolving an API call issue (missing URL parameters for traces), but no changes addressing URL parameter handling are visible in the diff. Please clarify if this functionality is handled elsewhere or update the description accordingly. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_3fsqwA5WjrbGeWRo
You can customize by changing your verbosity settings, reacting with π or π, replying to comments, or adding code review rules.
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.
lgtm
β¦ potential undefined values
91ee3e3
6903aa4
to
91ee3e3
Compare
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.
Important
Looks good to me! π
Reviewed 91ee3e3 in 42 seconds. Click for details.
- Reviewed
27
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
2
draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with π or π to teach Ellipsis.
1. frontend/src/pages/LogsExplorer/__tests__/LogsExplorer.test.tsx:232
- Draft comment:
The new test checks for the presence of auto refresh related buttons using accessible names (/sync/i and /caret-down/i). Consider extending the test to verify the actual auto refresh behavior (e.g., confirming that logs are refreshed when the button is activated), rather than just presence. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
2. frontend/src/pages/LogsExplorer/__tests__/LogsExplorer.test.tsx:232
- Draft comment:
The PR description mentions an issue regarding API calls missing URL params, yet this diff only adds tests for the auto refresh toolbar in Logs Explorer. Ensure that the API call parameter issue is addressed and properly tested elsewhere, as the current changes do not cover it. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
This comment is asking the PR author to ensure that an issue mentioned in the PR description is addressed and tested, which violates the rule against asking the author to ensure behavior is intended or tested. It does not provide a specific code suggestion or ask for a specific test to be written. Therefore, it should be removed.
Workflow ID: wflow_8oWQiF7iMzui1erK
You can customize by changing your verbosity settings, reacting with π or π, replying to comments, or adding code review rules.
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.
Important
Looks good to me! π
Reviewed 3463a52 in 1 minute and 49 seconds. Click for details.
- Reviewed
51
lines of code in4
files - Skipped
0
files when reviewing. - Skipped posting
6
draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with π or π to teach Ellipsis.
1. frontend/src/components/Graph/index.tsx:76
- Draft comment:
Removal of the 'MMM d, yyyy, h:mm:ss aaaa' mapping from formatMap: please confirm this is intentional and that no component requires this format. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% The comment violates several rules: 1) It's asking for confirmation of intention ("please confirm this is intentional") 2) It's speculative ("no component requires this format") 3) The code has a fallback format if the mapping isn't found 4) Understanding if this format is needed would require checking other files/components. The removal seems deliberate and the code handles missing formats gracefully. Maybe this format is critical for some component's functionality and removing it could cause subtle bugs in date formatting. The code explicitly handles missing formats with a fallback to ISO_DATETIME_SECONDS, so even if a component needs this format, it won't break - it will just use the fallback format. This comment should be deleted as it violates multiple rules - it asks for confirmation, is speculative, and the code handles missing formats gracefully.
2. frontend/src/container/QueryBuilder/components/Query/Query.tsx:326
- Draft comment:
Accessing aggregateAttribute.key without optional chaining assumes aggregateAttribute is always defined. Please ensure this invariant holds to prevent runtime errors. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
The comment is asking the author to ensure that a certain condition holds, which violates the rule against asking the author to ensure behavior is intended. It doesn't provide a specific suggestion or ask for a specific test, which would have been acceptable.
3. frontend/src/container/QueryBuilder/filters/QueryBuilderSearchV2/SpanScopeSelector.tsx:63
- Draft comment:
Removal of optional chaining in 'filters.some(...)' is acceptable as a default empty array is provided; just ensure that filters is never null. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
4. frontend/src/pages/TracesExplorer/index.tsx:78
- Draft comment:
Directly accessing 'query.groupBy.length' assumes 'groupBy' is always defined. Confirm that this guarantee holds to avoid potential runtime errors. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% The code is in a reducer that's operating on currentQuery.builder.queryData. Looking at the types, Query interface is imported. The change removes defensive programming that was there before. However, without seeing the Query type definition, I can't be 100% certain if groupBy is guaranteed to exist. The fact that the code previously had optional chaining suggests there may have been a reason for it. I don't have access to the Query type definition to verify if groupBy is required. The previous code's defensive programming could have been overly cautious rather than necessary. Given that this is a TypeScript file and the change was made deliberately to remove the optional chaining, it's likely the author verified that groupBy is guaranteed to exist in the Query type. The comment should be deleted. Without strong evidence that groupBy can be undefined and given that this is TypeScript code where the type system would catch such issues, we should trust the author's change to remove optional chaining.
5. frontend/src/pages/TracesExplorer/index.tsx:225
- Draft comment:
The addition of the 'showAutoRefresh' prop to DateTimeSelector appears to enable auto-refresh functionality. Please verify that DateTimeSelector properly handles this prop and that tests cover the auto-refresh behavior. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
6. frontend/src/pages/TracesExplorer/index.tsx:100
- Draft comment:
The PR description mentions fixing an issue where URL parameters were not passed to the API call. However, no explicit changes regarding URL params handling are visible in this diff. Please confirm that the fix for empty URL parameters is implemented. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_R2ptH8TEUct5wQdg
You can customize by changing your verbosity settings, reacting with π or π, replying to comments, or adding code review rules.
β¦iews of logs explorer
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.
Important
Looks good to me! π
Reviewed 0b31259 in 1 minute and 18 seconds. Click for details.
- Reviewed
117
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
5
draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with π or π to teach Ellipsis.
1. frontend/src/pages/LogsExplorer/__tests__/LogsExplorer.test.tsx:23
- Draft comment:
The updated useLocation mock now returns a long, hardcoded search string. Consider extracting this string into a named constant or helper to improve readability and maintainability. - Reason this comment was not posted:
Confidence changes required:50%
<= threshold50%
None
2. frontend/src/pages/LogsExplorer/__tests__/LogsExplorer.test.tsx:252
- Draft comment:
Good use of test.each to check that the auto refresh buttons are present across multiple panel types. Consider adding tests for the actual auto refresh behavior (e.g. triggering refresh events) in future iterations. - Reason this comment was not posted:
Confidence changes required:30%
<= threshold50%
None
3. frontend/src/pages/LogsExplorer/__tests__/LogsExplorer.test.tsx:260
- Draft comment:
The QueryBuilderContext provider values are repeated in the test setup. Consider refactoring these repeated properties into a helper or factory function to reduce duplication and improve clarity. - Reason this comment was not posted:
Confidence changes required:50%
<= threshold50%
None
4. frontend/src/pages/LogsExplorer/__tests__/LogsExplorer.test.tsx:1
- Draft comment:
The PR description mentions resolving an API call parameters issue on the traces page, but this diff only addresses auto refresh functionality in LogsExplorer. Please ensure that the API call parameter issue is also resolved or update the PR description accordingly. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
5. frontend/src/pages/LogsExplorer/__tests__/LogsExplorer.test.tsx:88
- Draft comment:
The helper function 'checkAutoRefreshButtonsPresent' uses regex matching on button names ('sync' and 'caret-down'), which could be brittle if the UI text changes. Consider using data-testid attributes for more robust and stable selectors in tests. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
Workflow ID: wflow_VM10gQMT1y6vhNv1
You can customize by changing your verbosity settings, reacting with π or π, replying to comments, or adding code review rules.
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.
Important
Looks good to me! π
Reviewed 8eb7149 in 1 minute and 18 seconds. Click for details.
- Reviewed
16
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
2
draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with π or π to teach Ellipsis.
1. frontend/src/pages/LogsExplorer/__tests__/LogsExplorer.test.tsx:21
- Draft comment:
The useLocation mock now only returns the pathname, with the 'search' property removed. Ensure this change doesn't inadvertently disable tests that rely on query parameters for API calls. If URL search parameters are needed for testing parameter passing, consider reintroducing or adapting these tests. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =50%
<= threshold50%
This comment is asking the author to ensure that the change doesn't disable tests, which violates the rule against asking the author to ensure behavior is intended or tested. However, it also provides a specific suggestion to reintroduce or adapt tests if needed, which is a valid suggestion.
2. frontend/src/pages/LogsExplorer/__tests__/LogsExplorer.test.tsx:250
- Draft comment:
The tests currently verify that auto refresh buttons are present. Consider also adding tests to verify the auto refresh interval behavior or that the auto refresh action is properly triggered on interaction, ensuring full coverage for the new feature. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_h7Aptm3efnKujkoC
You can customize by changing your verbosity settings, reacting with π or π, replying to comments, or adding code review rules.
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.
Current state
- I enable auto refresh
- I add a new filter but don't press
stage and run
- The auto refresh don't add the new filter in the subsequent request.
What should happen here, please come up with a solution/statement and why,
@nityanandagohain, would it be okay if I separately pick up the above, since the flow is the same in trace explorer as well, and needs to be handled for logs explorer and traces explorer. I have created this ticket: https://github.com/SigNoz/engineering-pod/issues/2572 |
β¦rences context provider
2407403
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.
Important
Looks good to me! π
Reviewed 2407403 in 49 seconds. Click for details.
- Reviewed
15
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
1
draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with π or π to teach Ellipsis.
1. frontend/src/pages/LogsExplorer/__tests__/LogsExplorer.test.tsx:321
- Draft comment:
Wrap with is now applied. Ensure this is intentional for tests that depend on preference context and remains consistent with other tests. If possible, consider using a custom render wrapper to reduce repetition. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
Workflow ID: wflow_b3TP7bJZbZl1SYcN
You can customize by changing your verbosity settings, reacting with π or π, replying to comments, or adding code review rules.
π Summary
Add auto refresh to logs explorer page
β Changes
π·οΈ Required: Add Relevant Labels
ex:
frontend
backend
devops
bug
enhancement
ui
test
π₯ Reviewers
π§ͺ How to Test
π Related Issues
Closes #6821
πΈ Screenshots / Screen Recording (if applicable / mandatory for UI related changes)
2025-05-21.18-10-05.mov
π Checklist
π Notes for Reviewers
Important
Adds auto-refresh feature to Logs Explorer with updated tests for all views.
LogsExplorer
inindex.tsx
for list, time series, and table views.LogsExplorer.test.tsx
to include tests for auto-refresh button presence in all views.checkAutoRefreshButtonsPresent
function to verify button presence.Toolbar
component usage inindex.tsx
to enableshowAutoRefresh
.This description was created by
for 2407403. You can customize this summary. It will automatically update as commits are pushed.