Skip to content
This repository has been archived by the owner on Jul 31, 2023. It is now read-only.

zpages: fix the count formatter argument type and StatsSnapshot field types #951

Closed
odeke-em opened this issue Oct 20, 2018 · 0 comments
Closed

Comments

@odeke-em
Copy link
Member

odeke-em commented Oct 20, 2018

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

zpages: executing template: template: rpcz:31:41: executing "rpcz" at <count>: wrong type for value; expected int64; got int

and in fact the test output also reported the runtime template error in
https://ci.appveyor.com/project/opencensusgoteam/opencensus-go/builds/19498802
screen shot 2018-10-20 at 3 53 53 pm
and
https://travis-ci.org/census-instrumentation/opencensus-go/builds/441324783#L1183
screen shot 2018-10-20 at 3 55 06 pm

The proper way to test those templates is by passing in a buffer and invoking execute then checking the output and errors such as

        if err := tmpl.Execute(buf, data); err != nil {
                t.Fatalf("Failed to execute template: %v", err)
        }

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.

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
@odeke-em odeke-em changed the title zpages: fix the count formatter argument type zpages: fix the count formatter argument type and StatsSnapshot field types Oct 20, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant