This repository has been archived by the owner on Jul 31, 2023. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 329
zpages: fix the count formatter argument type and StatsSnapshot field types #951
Comments
odeke-em
added a commit
that referenced
this issue
Oct 20, 2018
The signature for countFormatter's arguments was changed in PR #948 from int to int64 but unfortunately we forgot to update the int fields from int to the new type. Issue #948's recommendation was to use uint64 since that affords us more time to overflow e.g. even at 25QPs it'd take a couple of millenia to overflow with uint64. This change completes the change of types of the int variables but also adds tests that make use of the template functions and a snapshot and then compare the output or fail. Previously the tests complained but since they were HTTP tests not attached to an output buffer that could be inspected, the bug innocently crept away in standard output and the tests incorrectly passed as in: * https://ci.appveyor.com/project/opencensusgoteam/opencensus-go/builds/19498802#L700 * https://travis-ci.org/census-instrumentation/opencensus-go/builds/441324783#L1183 This problem was reported by an OpenCensus-Service user in * census-instrumentation/opencensus-service#123 Fixes #895 Fixes #951
odeke-em
added a commit
that referenced
this issue
Oct 20, 2018
The signature for countFormatter's arguments was changed in PR #948 from int to int64 but unfortunately we forgot to update the int fields from int to the new type. Issue #948's recommendation was to use uint64 since that affords us more time to overflow e.g. even at 25QPs it'd take a couple of millenia to overflow with uint64. This change completes the change of types of the int variables but also adds tests that make use of the template functions and a snapshot and then compare the output or fail. Previously the tests complained but since they were HTTP tests not attached to an output buffer that could be inspected, the bug innocently crept away in standard output and the tests incorrectly passed as in: * https://ci.appveyor.com/project/opencensusgoteam/opencensus-go/builds/19498802#L700 * https://travis-ci.org/census-instrumentation/opencensus-go/builds/441324783#L1183 This problem was reported by an OpenCensus-Service user in * census-instrumentation/opencensus-service#123 Fixes #895 Fixes #951
odeke-em
added a commit
that referenced
this issue
Oct 20, 2018
The signature for countFormatter's arguments was changed in PR #948 from int to int64 but unfortunately we forgot to update the int fields from int to the new type. Issue #948's recommendation was to use uint64 since that affords us more time to overflow e.g. even at 25QPs it'd take a couple of millenia to overflow with uint64. This change completes the change of types of the int variables but also adds tests that make use of the template functions and a snapshot and then compare the output or fail. Previously the tests complained but since they were HTTP tests not attached to an output buffer that could be inspected, the bug innocently crept away in standard output and the tests incorrectly passed as in: * https://ci.appveyor.com/project/opencensusgoteam/opencensus-go/builds/19498802#L700 * https://travis-ci.org/census-instrumentation/opencensus-go/builds/441324783#L1183 This problem was reported by an OpenCensus-Service user in * census-instrumentation/opencensus-service#123 Fixes #895 Fixes #951
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
PR #948 changed the signature of countFormatter's argument from int to int64 however the fields of Snapshot that are passed into count during template execution are still int. This issue was reported by a user of the opencensus-service as per census-instrumentation/opencensus-service#123
and in fact the test output also reported the runtime template error in


https://ci.appveyor.com/project/opencensusgoteam/opencensus-go/builds/19498802
and
https://travis-ci.org/census-instrumentation/opencensus-go/builds/441324783#L1183
The proper way to test those templates is by passing in a buffer and invoking execute then checking the output and errors such as
In the issue #895 I had recommended uint64, but we went with int64 which while still large, we have the ability to take advantage of the extra precision as I had described and also ensure graceful wrapping back to 0 on overflow in a couple of millenia
It would also be nice to make the zpages functions testable but also the formatters testable so that we can catch such innocent regressions early enough.
The text was updated successfully, but these errors were encountered: