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

expvar: document what expvar.Var's String() interface method requires you to return #15088

Closed
siebenmann opened this issue Apr 3, 2016 · 6 comments
Labels
FrozenDueToAge Suggested Issues that may be good for new contributors looking for work to do.
Milestone

Comments

@siebenmann
Copy link

Please answer these questions before submitting your issue. Thanks!

  1. What version of Go are you using (go version)?
    go version devel +b836fe6 Sat Apr 2 18:25:27 2016 -0400 linux/amd64
  2. What operating system and processor architecture are you using (go env)?
    linux/amd64

Currently, the expvar.Var interface looks like it is the well known fmt.Stringer interface; both have a single String() method in them. However, this is not the case, because expvar.Var's String() requires you to return a proper JSON object, with the correct quoting and all. This means that not all fmt.Stringer methods will do; for instance, time.Time's String() method returns an unquoted string that will cause JSON parsing failures. That means that naively writing something like:

m := expvar.NewMap("myapp")
m.Set("startTime", time.Now())

will cause mysterious downstream failures in things that attempt to consume expvar variables (because time.Time returns an unquoted string, which then winds up in the JSON for /debug/vars).

This programmer mistake could be somewhat avoided if the expvar documentation noted the JSON formatting requirement and also that String() functions in other packages that are intended to satisfy fmt.Stringer will often not work to satisfy expvar.Var.

@dgryski
Copy link
Contributor

dgryski commented Apr 5, 2016

@siebenmann
Copy link
Author

Is it worth explicitly mentioning that this is not necessarily compatible with fmt.Stringer and thus common .String() usage (either in the Var comments or in general package comments)? One can work it out from the current documentation for String(), but I'm not sure it's going to be clear or obvious to people.

(Specifically, I'm not sure I'd have realized that eg time.Time was not compatible from this documentation alone. But on the other hand, the Go documentation style tends towards terse.)

@bradfitz
Copy link
Contributor

bradfitz commented Apr 5, 2016

Or maybe it can use it if it's valid JSON, else fall back to escaping it as a string.

/cc @adg

@adg
Copy link
Contributor

adg commented Apr 6, 2016

Or maybe it can use it if it's valid JSON, else fall back to escaping it as a string.

That seems like a fine solution to me.

@bradfitz bradfitz added this to the Go1.7 milestone Apr 7, 2016
@bradfitz bradfitz added help wanted Suggested Issues that may be good for new contributors looking for work to do. and removed help wanted labels Apr 7, 2016
@gopherbot
Copy link

CL https://golang.org/cl/23068 mentions this issue.

@gopherbot
Copy link

CL https://golang.org/cl/23232 mentions this issue.

@golang golang locked and limited conversation to collaborators May 19, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge Suggested Issues that may be good for new contributors looking for work to do.
Projects
None yet
Development

No branches or pull requests

5 participants