newrelic / newrelic-client-go

New Relic Client for the Go programming language
https://newrelic.github.io/observability-as-code
Apache License 2.0
73 stars 89 forks source link

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

Closed pranav-new-relic closed 4 weeks ago

pranav-new-relic commented 1 month ago

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 commented 4 weeks ago
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.