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: UTC timezone #1107

Merged
merged 9 commits into from
Jun 1, 2022
Merged

feat: UTC timezone #1107

merged 9 commits into from
Jun 1, 2022

Conversation

pavelpashkovsky
Copy link
Contributor

@pavelpashkovsky pavelpashkovsky commented May 19, 2022

Adding possibility to watch profiling data time in local and UTC timezone (#1084)

DEMO: (https://monosnap.com/file/IBcMlw4Pjfpg6rvx483WDv1CuBJLGh)

Pyrosope uses UNIX timestamp for all time operations (UNIX timestamp = UTC time). This POC added posibility to watch time without affecting parts working on calculating time. User will be able to select between browser(local) timezone and UTC which time he want to see on the UI

@github-actions
Copy link
Contributor

github-actions bot commented May 19, 2022

size-limit report 📦

Path Size Loading time (3g) Running time (snapdragon) Total time
webapp/public/assets/app.js 463 KB (+0.12% 🔺) 9.3 s (+0.12% 🔺) 2.2 s (+3.16% 🔺) 11.5 s

@codecov
Copy link

codecov bot commented May 19, 2022

Codecov Report

Merging #1107 (bf0c87c) into main (a35d562) will increase coverage by 0.01%.
The diff coverage is 75.93%.

@@            Coverage Diff             @@
##             main    #1107      +/-   ##
==========================================
+ Coverage   70.79%   70.80%   +0.02%     
==========================================
  Files          93       94       +1     
  Lines        3022     3047      +25     
  Branches      757      761       +4     
==========================================
+ Hits         2139     2157      +18     
- Misses        879      886       +7     
  Partials        4        4              
Impacted Files Coverage Δ
...raphComponent/DiffLegendPaletteDropdown.module.css 61.54% <ø> (ø)
...e-flamegraph/src/FlameGraph/FlameGraphRenderer.tsx 52.46% <0.00%> (ø)
...raph/src/FlameGraph/FlamegraphRenderer.module.scss 61.54% <ø> (ø)
...pyroscope-flamegraph/src/ProfilerHeader.module.css 61.54% <ø> (ø)
...ebapp/javascript/components/ExportData.module.scss 61.54% <ø> (ø)
webapp/javascript/ui/Modals/Modal.module.css 61.54% <0.00%> (ø)
webapp/javascript/ui/Select.tsx 100.00% <ø> (ø)
webapp/sass/flamegraph.scss 30.77% <ø> (ø)
webapp/javascript/redux/reducers/ui.ts 63.64% <50.00%> (-6.73%) ⬇️
.../FlameGraphComponent/DiffLegendPaletteDropdown.tsx 73.59% <63.64%> (ø)
... and 19 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 310c812...bf0c87c. Read the comment docs.

@eh-am
Copy link
Collaborator

eh-am commented May 20, 2022

Haven't looked at the code yet but here are my thoughts:

  • would be useful to be able to pick another option other than utc/local, for example even though i work in brazil ( - 3) we often talk in pst (-7 i think?)
  • is this setting global? Eg all dates within the app would be following the setting, and if it's global isn't it weird for this setting to be under the timepicker component?
  • is this setting persisted? Browser only (local storage)?
  • this is more of a reflection, when sharing profiles, do people want to send them in a given format (eg utc) or not?

@pavelpashkovsky
Copy link
Contributor Author

@eh-am

is this setting global? Eg all dates within the app would be following the setting, and if it's global...

This setting is global for all pages of Pyroscope web app. It's important to understand that I updated only the "front-end" of interacting with time. This PR doesn't affect functions which set/calculate time, it's only for fuctions which "render" time (time selectors/ timelines etc.). Pyroscope already stores and works with time in unix format what equals to UTC.

...isn't it weird for this setting to be under the timepicker component?

You are right, it's just for POC. Can you describe your vision of this selector?

is this setting persisted? Browser only (local storage)?

yes it is, I keep this param in redux state

this is more of a reflection, when sharing profiles, do people want to send them in a given format (eg utc) or not?

no, sharing reports logic stays the same. recepient will see this report in timezone which he set on his browser

@pavelpashkovsky pavelpashkovsky marked this pull request as ready for review May 24, 2022 08:36
Copy link
Collaborator

@eh-am eh-am left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm slightly confused by why we need to maintain a isUTC, isn't utc == gmt and therefore, offset 0?

Couldn't some of the code be simplified by simply using an offset? Then if we ever implement support for other timezones it's just a matter of taking different offsets? [0]

[0] Althought I still feel like offsets are not the best abstraction, since offset change in eg daylight savings.

^^^^^^^^^
based on this i think the abstractions should be timezones directly no?

webapp/javascript/hooks/timeZone.hook.ts Outdated Show resolved Hide resolved
webapp/javascript/components/TimelineChartWrapper.tsx Outdated Show resolved Hide resolved
@pavelpashkovsky
Copy link
Contributor Author

pavelpashkovsky commented May 26, 2022

@eh-am agree with you. In fact we operate time offset, not UTC/local time, I refactored my solution in a bit in accordance with your ideas.

...Then if we ever implement support for other timezones...

But in case you want to add support for all timezones in future, there might be difficulties with refactor timelines, because Flot timeline supports only 2 options - utc and local time (might need serious refactor)

UPD: not relevant

@pavelpashkovsky pavelpashkovsky changed the title feat: UTC timezone [WIP] feat: UTC timezone May 27, 2022
@Rperry2174 Rperry2174 merged commit 9fa550c into main Jun 1, 2022
@Rperry2174 Rperry2174 deleted the utc-timezone branch June 1, 2022 17:39
@Rperry2174
Copy link
Contributor

Thanks @pavelpashkovsky !! Looks great :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants