-
Notifications
You must be signed in to change notification settings - Fork 711
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
TraceEvent: expose timestamps in UTC format #1740
base: main
Are you sure you want to change the base?
Conversation
Under heavy system load (10K+ events per second), timezone conversion accounts for the vast majority of time when reading the timestamp. In case the client does not need local time, this new property saves a considerable amount of time. Signed-off-by: Vitaly Chipounov <vitaly@cyberhaven.com>
@vitalych please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.
Contributor License AgreementContribution License AgreementThis Contribution License Agreement (“Agreement”) is agreed to by the party signing below (“You”),
|
Is there more to this PR? I mean, I see where TimeStampUtc, is introduced which doesn't include the local timezone conversion, but where is this new property being consumed? |
That's all there is to this PR. This property is to be used by clients that link with the library. The screenshot would show our own code above the get_Timestamp method. For the context, we have an application that gets a stream of TraceEvent objects and needs to get timestamps quickly. The workaround we have is to get TimeStampQPC and convert it to UTC ourselves using the same logic that the library uses internally. I'd be happy to add test cases, let me know where they should go. |
@vitalych the command you issued was incorrect. Please try again. Examples are:
and
|
@microsoft-github-policy-service agree company="Cyberhaven, Inc" |
@vitalych the command you issued was incorrect. Please try again. Examples are:
and
|
This seems reasonable to me. Having some tests in https://github.com/microsoft/perfview/tree/main/src/TraceEvent/TraceEvent.Tests would be a good thing. Probably just create a new @pharring, do you have concerns with this? From my perspective, this just seems like goodness, and I don't see any concerns around not being able to implement all of these new UTC properties for all current and future formats. |
I've no concerns. Once this PR is in, you could consider:
|
Under heavy system load (10K+ events per second), timezone conversion accounts for the vast majority of time when reading the timestamp. In case the client does not need local time, the new properties saves a considerable amount of time.