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

Update EventPropertiesDecorator to respect the value set by SetTimestamp #909

Merged
merged 2 commits into from
Jul 30, 2021

Conversation

kindbe
Copy link
Contributor

@kindbe kindbe commented Jul 30, 2021

EventPropertiesDecorator ignores any value specified in EventProperties::SetTimestamp and all records get the timestamp value set in BaseDecorator (PAL::getUtcSystemTimeinTicks()). This change updates EventPropertiesDecorator::decorate to set record.time to eventProperties.GetTimestamp() if it is non-zero.

lib/decorators/EventPropertiesDecorator.hpp Outdated Show resolved Hide resolved
Copy link
Contributor

@mkoscumb mkoscumb left a comment

Choose a reason for hiding this comment

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

Any test updates with this?

@kindbe kindbe merged commit 9f7c580 into master Jul 30, 2021
@kindbe kindbe deleted the dabrow/SetTimestamp branch July 30, 2021 18:41
@maxgolov
Copy link
Contributor

Any test updates with this?

We have some tests that simply validate the record timestamp. Maybe we can add a few lines / an extra test in that same test, that sets the timestamp 1 hour behind, and verify that it matches.

@kindbe
Copy link
Contributor Author

kindbe commented Jul 30, 2021

Any test updates with this?

We have some tests that simply validate the record timestamp. Maybe we can add a few lines / an extra test in that same test, that sets the timestamp 1 hour behind, and verify that it matches.

Drat, I missed Matt's comment prior to hitting the button. I had looked for EventPropertiesDecorator tests but didn't find any. @maxgolov if you can point me in the right direction, I can look at updating the record test(s).

@maxgolov
Copy link
Contributor

maxgolov commented Jul 30, 2021

@kindbe - something around here, and/or maybe a callback similar to that one:

EXPECT_EQ(record.time, int64_t(recordTimeTicks.ticks) );

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