feat: apply timezone to the date/time in tabular data throughout the app #6565
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
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> |
ahmadshaheer
changed the base branch from
fix/open-timepicker-after-closing-the-timezone-picker
to
fix/make-the-x-axis-and-tooltip-time-format-consistent
|
Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id> |
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
👍 Looks good to me! Reviewed everything up to 614823a in 1 minute and 9 seconds
More details
- Looked at
1072lines of code in25files - Skipped
0files when reviewing. - Skipped posting
10drafted comments based on config settings.
1. frontend/src/hooks/useTimezoneFormatter/useTimezoneFormatter.ts:24
- Draft comment:
Consider adding a default value foruserTimezoneto ensure that the hook functions correctly even if no timezone is provided. This can help prevent potential errors or unexpected behavior. - Reason this comment was not posted:
Confidence changes required:20%
The PR introduces a new hookuseTimezoneFormatterto format timestamps according to the user's timezone. This hook is used throughout the application to replace direct usage ofdayjsfor formatting timestamps. The hook also includes a caching mechanism to improve performance by avoiding redundant calculations for the same input. The caching mechanism is implemented using a Map and includes logic to clear expired entries when the cache size exceeds a certain limit. The hook is integrated into theTimezoneProvider, and theformatTimestampfunction is exposed via theuseTimezonehook. This change is applied consistently across various components and containers in the application, ensuring that all timestamps are formatted according to the user's selected timezone. The PR appears to be well-implemented, with attention to performance and consistency across the codebase. However, there are a few areas where improvements can be made, such as ensuring that the cache cleanup logic is efficient and does not introduce unnecessary complexity. Overall, the PR is a positive step towards improving the user experience by ensuring that timestamps are displayed in the user's preferred timezone.
2. frontend/src/hooks/useTimezoneFormatter/useTimezoneFormatter.ts:55
- Draft comment:
Consider adding error handling for invalid timestamp inputs to prevent potential runtime errors. This can help ensure that the application remains stable even if unexpected input is provided. - Reason this comment was not posted:
Confidence changes required:20%
The PR introduces a new hookuseTimezoneFormatterto format timestamps according to the user's timezone. This hook is used throughout the application to replace direct usage ofdayjsfor formatting timestamps. The hook also includes a caching mechanism to improve performance by avoiding redundant calculations for the same input. The caching mechanism is implemented using a Map and includes logic to clear expired entries when the cache size exceeds a certain limit. The hook is integrated into theTimezoneProvider, and theformatTimestampfunction is exposed via theuseTimezonehook. This change is applied consistently across various components and containers in the application, ensuring that all timestamps are formatted according to the user's selected timezone. The PR appears to be well-implemented, with attention to performance and consistency across the codebase. However, there are a few areas where improvements can be made, such as ensuring that the cache cleanup logic is efficient and does not introduce unnecessary complexity. Overall, the PR is a positive step towards improving the user experience by ensuring that timestamps are displayed in the user's preferred timezone.
3. frontend/src/hooks/useTimezoneFormatter/useTimezoneFormatter.ts:64
- Draft comment:
Consider adding a fallback mechanism for formatting timestamps in case thedayjslibrary fails to format the input. This can help ensure that the application remains functional even if there are issues with the library. - Reason this comment was not posted:
Confidence changes required:20%
The PR introduces a new hookuseTimezoneFormatterto format timestamps according to the user's timezone. This hook is used throughout the application to replace direct usage ofdayjsfor formatting timestamps. The hook also includes a caching mechanism to improve performance by avoiding redundant calculations for the same input. The caching mechanism is implemented using a Map and includes logic to clear expired entries when the cache size exceeds a certain limit. The hook is integrated into theTimezoneProvider, and theformatTimestampfunction is exposed via theuseTimezonehook. This change is applied consistently across various components and containers in the application, ensuring that all timestamps are formatted according to the user's selected timezone. The PR appears to be well-implemented, with attention to performance and consistency across the codebase. However, there are a few areas where improvements can be made, such as ensuring that the cache cleanup logic is efficient and does not introduce unnecessary complexity. Overall, the PR is a positive step towards improving the user experience by ensuring that timestamps are displayed in the user's preferred timezone.
4. frontend/src/components/Logs/RawLogView/index.tsx:98
- Draft comment:
Avoid using hardcoded color values. Use design tokens or predefined color constants instead. This issue is also present in other files. - Reason this comment was not posted:
Comment was not on a valid diff hunk.
5. frontend/src/components/Logs/TableView/useTableView.tsx:86
- Draft comment:
Avoid using hardcoded color values. Use design tokens or predefined color constants instead. This issue is also present in other files. - Reason this comment was not posted:
Marked as duplicate.
6. frontend/src/components/ResizeTable/TableComponent/Time.tsx:7
- Draft comment:
Avoid using hardcoded color values. Use design tokens or predefined color constants instead. This issue is also present in other files. - Reason this comment was not posted:
Marked as duplicate.
7. frontend/src/container/AlertHistory/Timeline/Table/Table.tsx:43
- Draft comment:
Avoid using hardcoded color values. Use design tokens or predefined color constants instead. This issue is also present in other files. - Reason this comment was not posted:
Marked as duplicate.
8. frontend/src/container/AlertHistory/Timeline/Table/useTimelineTable.tsx:82
- Draft comment:
Avoid using hardcoded color values. Use design tokens or predefined color constants instead. This issue is also present in other files. - Reason this comment was not posted:
Marked as duplicate.
9. frontend/src/container/AllError/index.tsx:290
- Draft comment:
Avoid using hardcoded color values. Use design tokens or predefined color constants instead. This issue is also present in other files. - Reason this comment was not posted:
Marked as duplicate.
10. frontend/src/container/ErrorDetails/index.tsx:137
- Draft comment:
Avoid using hardcoded color values. Use design tokens or predefined color constants instead. This issue is also present in other files. - Reason this comment was not posted:
Marked as duplicate.
Workflow ID: wflow_iOGxRTp2CFORkaBs
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
|
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.
👍 Looks good to me! Incremental review on a7da3f3 in 31 seconds
More details
- Looked at
630lines of code in23files - Skipped
0files when reviewing. - Skipped posting
2drafted comments based on config settings.
1. frontend/src/components/Logs/ListLogView/index.tsx:174
- Draft comment:
Consider simplifying the function nameformatTimezoneAdjustedTimestampto improve readability and maintainability. A shorter name likeformatTimestamporformatWithTimezonecould be more concise and still convey the intended functionality. This suggestion applies to all instances whereformatTimezoneAdjustedTimestampis used. - Reason this comment was not posted:
Confidence changes required:50%
The PR replaces the function nameformatTimestampwithformatTimezoneAdjustedTimestampacross multiple files. This change is consistent and aligns with the PR description. However, the new function name is quite long and could be simplified for better readability and maintainability.
2. frontend/src/container/ListOfDashboard/DashboardsList.tsx:396
- Draft comment:
Avoid using hardcoded color values. Use design tokens or predefined color constants instead. This issue is also present in other files. - Reason this comment was not posted:
Comment was not on a valid diff hunk.
Workflow ID: wflow_qb7uqU1frUG7XyNY
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
|
Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id> |
ahmadshaheer
changed the base branch from
fix/make-the-x-axis-and-tooltip-time-format-consistent
to
feat/timezone-formatter-custom-hook-and-adjust-graphs-timezone
|
Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id> |
…gs explorer -> list view)
…logs explorer -> list view)
… (logs explorer -> list view)
…of alert history page
ahmadshaheer
force-pushed
the
feat/apply-timezone-to-tabular-data-throughout-the-app
branch
from
a7da3f3
to
0a21911
Compare
|
Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id> |
| return { formatTimezoneAdjustedTimestamp }; | ||
| } | ||
|
|
||
| export default useTimezoneFormatter; |
There was a problem hiding this comment.
@ahmadshaheer - for this check for comments and suggestion added here in this PR - #6566
There was a problem hiding this comment.
Done, @SagarRajput-7 👍
SagarRajput-7
approved these changes
Dec 2, 2024
There was a problem hiding this comment.
@ahmadshaheer - under this too many files and consumers are changed, hoping that we have tested things throughly. Code-wise things look good
… page when groupe filter is applied (#6570) * feat: add timezone support to triggered alerts page when groupe filter is applied * feat: apply timezone adjustment to ingestion settings
#6566) * feat: display timezone adjusted time range in time picker * fix: make x axis and tooltip time format consistent in uPlot graphs * fix: open the timepicker on clicking the input after closing the timezone picker by clicking outside * feat: custom hook for formatting timezone * feat: add timezone support to traces explorer timestamp column * feat: add timezone support to saved views * chore: improve timezone formatter custom hook * feat: add support for timezone adjusted timestamp in raw log view (logs explorer -> list view) * feat: add support for timezone adjusted timestamp in log table view (logs explorer -> list view) * feat: add support for timezone adjusted timestamp in log default view (logs explorer -> list view) * feat: add support for timezone adjusted timestamp in log details side pane * feat: add support for timezone adjusted timestamp in pipeline pages * feat: add support for timezone in dashboard list * feat: add support for timezone adjusted created/updated at in alert rules list page * feat: add support for timezone adjusted created at in timeline table of alert history page * feat: add support for timezone adjusted Firing Since in triggered alerts page * feat: add support for timezone adjusted Timestamp for List Panel of dashboard * feat: add support for timezone adjusted First/Last Seen in exceptions list page * feat: add support for timezone adjusted date in exception details page * feat: add support for timezone adjusted Valid From/To in History tab of Licences * chore: rename formatTimestamp -> formatTimezoneAdjustedTimestamp * feat: add timezone support to logs explorer chart (built with Chartjs) * chore: remove unnecessary chartjs-adapter-date-fns import * chore: improve cache key * chore: make clearCacheEntries DRYer * chore: add docstring to formatTimezoneAdjustedTimestamp
ahmadshaheer
merged commit 638f257
into
feat/timezone-formatter-custom-hook-and-adjust-graphs-timezone
6 of 7 checks passed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Apply timezone support to Tabular Data
Traces explorer
Logs explorer
useTableView) [note: I checked all the columns, it seems that we don't get timestamp for any other column, added timezone support to the timestamp column]ListLogView)Dashboard
Alerts
Exceptions
List
Details page
Licenses
Related Issues / PR's
Part of https://github.com/SigNoz/engineering-pod/issues/2005
Screenshots
NA
Affected Areas and Manually Tested Areas
Important
Introduces timezone-based formatting for date/time displays across the app using a new
formatTimestampfunction.formatTimestampfunction inuseTimezonehook for timezone-based timestamp formatting.dayjsformatting withformatTimestampin components likeListLogView,RawLogView,TableView, and others.formatTimestamp.CustomTimePicker,LogsPanelComponent,TracesTableComponent,SaveView, and others now useformatTimestampfor date/time display.useTimezoneFormatterhook added to handle timezone-based formatting with caching.dayjsusage for timestamp formatting in favor offormatTimestamp.TimezoneProviderto includeformatTimestampin its context.This description was created by
for a7da3f3. It will automatically update as commits are pushed.