Skip to content

Support Option for Chrome Trace Unit to be in Nanoseconds #1074

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

mcalman
Copy link
Contributor

@mcalman mcalman commented Apr 11, 2025

PR #923 changed the default chrome trace unit to be ms. This was done to make readability easier, however, it has the following adverse side effects:

  1. Activities that are too close together are incorrectly treated by Chrome Trace-Viewer as nested and moved to separate lines. A discussion on the cause of this can be found here. Setting "displayTimeUnit": "ns" resolves this issue

  2. Activities with very short durations round to durations of 0.000 ms when using ms

Simple illustration of issue with "displayTimeUnit": "ms" (ms.pt.trace.json):
image

Issues resolved by switching to "displayTimeUnit": "ns" (ns.pt.trace.json):
image

@sraikund16
Copy link
Contributor

Unfortunately, our internal users have voiced that they prefer the default to be ms. Can you instead introduce a flag or env var to set this?

@mcalman mcalman changed the title Revert Default Chrome Trace Unit to be in Nanoseconds Support Option for Chrome Trace Unit to be in Nanoseconds Apr 15, 2025
@mcalman
Copy link
Contributor Author

mcalman commented Apr 15, 2025

@sraikund16 I have updated the PR to keep the default as ms and only switch to ns if the build flag -DDISPLAY_TRACE_IN_NS is set.

@facebook-github-bot
Copy link
Contributor

@sraikund16 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

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

Successfully merging this pull request may close these issues.

3 participants