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

v1.35.0 breaks any program trying to use a -v flag #1153

Closed
marcind opened this issue Jan 28, 2022 · 5 comments · Fixed by #1774
Closed

v1.35.0 breaks any program trying to use a -v flag #1153

marcind opened this issue Jan 28, 2022 · 5 comments · Fixed by #1774

Comments

@marcind
Copy link
Contributor

marcind commented Jan 28, 2022

The following simple program breaks when dd-trace-go is upgraded to v1.35.0

package main

import (
	"flag"
	"os"

	"gopkg.in/DataDog/dd-trace-go.v1/ddtrace/tracer"
)

func main() {
	var v bool
	flag.BoolVar(&v, "v", false, "make verbose")
	flag.Parse()

	if v {
		println("Hello world")
	}

	if os.Getenv("DD_AGENT_HOST") != "" {
		tracer.Start()
		defer tracer.Stop()
	}
}

The output when executing go run main.go is something like

/var/folders/yg/7bk34drs0zjfsb9b_r_1sykr0000gn/T/go-build2940344100/b001/exe/main flag redefined: v
panic: /var/folders/yg/7bk34drs0zjfsb9b_r_1sykr0000gn/T/go-build2940344100/b001/exe/main flag redefined: v

goroutine 1 [running]:
flag.(*FlagSet).Var(0xc000078180, {0x14e0a78, 0xc000026658}, {0x1450a20, 0x1}, {0x145389a, 0xc})
        /go/src/flag/flag.go:879 +0x2f4
flag.BoolVar(...)
        /go/src/flag/flag.go:638
main.main()
        /repro/main.go:12 +0x70
exit status 2

This has been traced to the usage of the github.com/golang/glog package (which also uses the -v flag)

Glog is imported via the following chain

go mod why github.com/golang/glog
# github.com/golang/glog
example.com/repro
gopkg.in/DataDog/dd-trace-go.v1/ddtrace/tracer
github.com/DataDog/datadog-agent/pkg/obfuscate
github.com/dgraph-io/ristretto
github.com/dgraph-io/ristretto/z
github.com/golang/glog

The breaking change was introduced in #1069 (committed in f55a622).

Rolling back to the previous commit by executing go get gopkg.in/DataDog/dd-trace-go.v1/ddtrace/tracer@11c835cb0 && go mod tidy makes the above-mentioned program work again.

It is not my expectation that some transitive dependency of a transitive dependency would cause applications to break because some library thinks it knows better and decides to hijack a bunch of flags (including something as generic as -v).

@marcind
Copy link
Contributor Author

marcind commented Jan 31, 2022

For what it's worth it appears that Google has no intention of fixing the underlying problem and libraries should just avoid depending on the glog package.

@felixge
Copy link
Member

felixge commented Feb 1, 2022

@marcind sorry you're hitting this issue. I think it's reasonable to expect that upgrading dd-trace-go doesn't break your application.

That being said, it's a tricky situation. glog seems difficult to prune from our dependency graph without giving up on ristretto. We also can't use a replace directive to point to a fork from within dd-trace-go since that only works for the main module according to the module spec. @marcind do you see any elegant workarounds for us?

On your end it seems like you should be able to use a FlagSet rather than relying on pkg-global state as a workaround?

cc @gbbr who worked on the bits related to glog

@marcind
Copy link
Contributor Author

marcind commented Feb 1, 2022

@felixge seems like the easiest might be to lobby for dgraph-io/ristretto#292 and/or dgraph-io/ristretto#293 as it appears dd-trace-go is not the only project affected by glog.

Outside of that, have you considered not using ristretto?

@radykal-com
Copy link
Contributor

This can now be fixed by updating github.com/DataDog/datadog-agent/pkg/obfuscate version. As per this PR DataDog/datadog-agent#15005 they replaced the ristretto dependency with a maintained fork.

Any chance to fix it?

@felixge
Copy link
Member

felixge commented Mar 17, 2023

@radykal-com the problem should be fixed in main now, and will be released soon (ETA end of month, maybe early next month).

Sorry this took so long and for the problems it has caused.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants