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

fix(changetracking): alter the functioning of milli and nanoseconds in timestamps #1151

Merged
merged 4 commits into from
Jun 6, 2024

Conversation

pranav-new-relic
Copy link
Member

@pranav-new-relic pranav-new-relic commented May 30, 2024

Background

A bug has been reported recently via New Relic support, in which it has been stated that upon specifying timestamps which are NOT time.Now() with the changetracking package, such as the following

Timestamp:      nrtime.EpochMilliseconds(time.Date(2024, 06, 03, 10, 53, 0, 0, time.Local)),

instead of

Timestamp:        nrtime.EpochMilliseconds(time.Now()),

seem to throw an error that states that the API has failed to validate the timestamp sent, as it is not within 24 hours of the current time.

Upon further investigation, the trend causing this error was identified: if the value of nanoseconds is 0, an error is thrown, and the error does not seem to be thrown with non zero values of nanoseconds, until the specified timestamp is out of 24 hours of the current time.

This is because it appears that the API does not take timestamps with 0 milliseconds (as milliseconds cannot be a null value). Because of the absence of a field that can help specify milliseconds in Golang native types (time.Time), the implementation of the datatype nrtime.EpochMilliseconds() relies on the value of nanoseconds to calculate milliseconds, which would be sent to the API. As a result, if nanoseconds is 0, milliseconds is subsequently 0 too, causing the error.

In order to fix this error, the first approach adopted was #1137, proposing a fix to the intrinsic behaviour of the nrtime.EpochMilliseconds datatype to have non zero milliseconds when nanoseconds is zero; however, this seemed to break the functioning of a multitude of other functions which use this datatype.

To avoid such a breaking change, the other solution adopted (#1139) was to duplicate nrtime.EpochMilliseconds specific to the changetracking package (to have changetracking.EpochMilliseconds), which would then comprise the implementation already defined in #1137, so that the change would apply only to changetracking. However, it was later realised that this would amount to a breaking change (for those who are using the Go Client directly and invoking functions in the changetracking package).

As a result, a final workaround that has been adopted is the current PR, in which the value of milliseconds sent to the API is made non zero (when nanoseconds is zero), by adding a value to nanoseconds, when it is found to be zero. This has been added inside the function which calls the NerdGraph mutation, and is hence, vulnerable to removal upon tutone generate, but should not be discarded.

Test cases have been added to validate the same.

@pranav-new-relic pranav-new-relic changed the title TBD fix(changetracking): alter the functioning of milli and nanoseconds in timestamps May 31, 2024
@pranav-new-relic pranav-new-relic force-pushed the changetracking-timestamp-fix-final branch from 0c774ca to afea6a0 Compare June 3, 2024 10:28
@pranav-new-relic pranav-new-relic force-pushed the changetracking-timestamp-fix-final branch from b6f4019 to 724693d Compare June 5, 2024 05:29
@pranav-new-relic
Copy link
Member Author

image

The only test failure seen is with alert policy channels, apparently caused by intermittent API issues which the alerts team has been notified of, and is investigating into.

This PR is good to be merged.

@pranav-new-relic pranav-new-relic marked this pull request as ready for review June 6, 2024 07:19
@pranav-new-relic pranav-new-relic merged commit 848f298 into main Jun 6, 2024
13 of 14 checks passed
@pranav-new-relic pranav-new-relic deleted the changetracking-timestamp-fix-final branch June 6, 2024 07:20
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.

3 participants