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

Jump to bottom

expvar: make it possible to remove memstat and cmdline defaults #29105

Open
as opened this issue Dec 5, 2018 · 4 comments
Open

expvar: make it possible to remove memstat and cmdline defaults #29105

as opened this issue Dec 5, 2018 · 4 comments
Labels
FeatureRequest NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone
Backlog

Comments

@as
Copy link
Contributor

as commented Dec 5, 2018

The expvar package contains an init function that registers the memstats and cmdline vars to the debug/vars http handler. The package does not provide a means to remove these defaults, forcing the consumer to deal with them. It would be nice if the user had a means to remove these defaults without resorting to unsafe or forking the package.

A benchmark I've added reveals a mild but unnecessary performance impact with the defaults (when the number of total expvar.Vars is expected to be small), but the real problem I think is aesthetic: My other vars have nothing to do with memory usage or the command line, so the memory profile is intrusive to the application.

We also can't use memstats and cmdline as named for our own purposes. This does not impact me personally.

goos: windows
goarch: amd64
pkg: expvar
BenchmarkExpvarHandler/cmdline.memstats-8               200000          7219 ns/op
BenchmarkExpvarHandler/none-8                            5000000           278 ns/op
BenchmarkExpvarHandler/user.10.vars-8                     300000          5130 ns/op
PASS
ok      expvar    4.822s

func BenchmarkExpvarHandler(b *testing.B) {
    for _, tc := range []struct {
        Name string
        Init func()
    }{
        {"cmdline.memstats", func() {}},
        {"none", func() { RemoveAll() }},
        {"user.10.vars", func() { for i := 0; i < 10; i++ { NewInt(fmt.Sprintf("i%d", i)) } }},
    } {
        tc.Init()
        b.Run(tc.Name, func(b *testing.B) {
            for n := 0; n < b.N; n++ {
                expvarHandler(writer{}, nil)
            }
        })
    }
}

type writer http.Header
func (w writer) WriteHeader(_ int)           {}
func (w writer) Write(p []byte) (int, error) { return len(p), nil }
func (w writer) Header() http.Header         { return http.Header(w) }

Interestingly, the package tests contain an unexported RemoveAll function. The recently-closed issue #27555 discusses it in detail, but does not mention the potential use for removing the default vars.
@as as changed the title expvar: make it possible to remove memory and command line defaults expvar: make it possible to remove memstat and cmdline defaults Dec 5, 2018
@agnivade agnivade added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Dec 11, 2018
@agnivade agnivade added this to the Go1.13 milestone Dec 11, 2018
@agnivade
Copy link
Contributor

/cc @bradfitz

@andybons andybons modified the milestones: Go1.13, Go1.14 Jul 8, 2019
@rsc rsc modified the milestones: Go1.14, Backlog Oct 9, 2019
@artemyarulin artemyarulin mentioned this issue Oct 10, 2019
Closed
@gopherbot
Copy link

Change https://golang.org/cl/200319 mentions this issue: expvar: make possible to delete all exported variables

@fgm
Copy link

fgm commented Jun 18, 2022 •
edited

While this issue is still open, changelog 200319 is marked abandoned and I cannot see any explanation either here or on https://golang.org/cl/200319 and PR #34832 was closed.

Was it just abandoned as old, or actually rejected ? It was even part of the 1.13, then 1.14, milestones before @rsc removed it from there.

That tiny patch moving RemoveAll from test to main code solves the automatic publishing of cmdline and memstats and provides a more general solution to reset the expvar configuration. Is there anything I can do to make this issue/CL live again ?

@ianlancetaylor
Copy link
Contributor

The change was closed because it was old. That was a mistake that should not have happened. Once the change was closed, the GitHub pull request was closed, and the GitHub repo was deleted. So you, or somebody, would have to recreate the change from the data that is still in Gerrit. Sorry about that.

That said, the change introduces new API, and our usual process for that is a proposal, as outlined at https://go.dev/s/proposal-process. We can open a new issue, or change this issue into a proposal for the change, as you prefer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
FeatureRequest NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Milestone
Backlog
Development

Successfully merging a pull request may close this issue.

expvar: make possible to delete all exported variables
8 participants
@bradfitz @rsc @fgm @andybons @agnivade @ianlancetaylor @as @gopherbot