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

Reconsider the dependency on OpenCensus #279

Closed
yegle opened this issue Aug 24, 2019 · 4 comments
Closed

Reconsider the dependency on OpenCensus #279

yegle opened this issue Aug 24, 2019 · 4 comments
Labels
NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.

Comments

@yegle
Copy link
Contributor

yegle commented Aug 24, 2019

The dependency on OpenCensus implicitly pulled in https://github.com/Shopify/sarama.

We should reconsider if the dependency on OpenCensus is necessary.

@travisgroth
Copy link
Contributor

Hey @yegle - the instrumentation (metrics and tracing) is built on OC, so we do need it.

The kafka client is...surprising. I'm guessing it's a (or part of a?) supported export format.

@desimone
Copy link
Contributor

Hi @yegle !

We try hard to keep dependencies as limited as possible so thanks for brining this up.

So my understanding is -- and unfortunately I can't find the documentation definitively saying so -- is that dependencies pulled from go.sum and go.mod where not explicitly marked direct, are pulled but not linked in the actual binary. So while it's a bit annoying to have to pull down the additional packages, I don't believe they are being linked unless we explicitly use that exporter.

 → go mod why -m github.com/Shopify/sarama
# github.com/Shopify/sarama
(main module does not need module github.com/Shopify/sarama)

vs

→ go mod why -m github.com/prometheus/client_golang
# github.com/prometheus/client_golang
github.com/pomerium/pomerium/internal/telemetry/metrics
github.com/prometheus/client_golang/prometheus

Direct dependencies:

go list -f '{{if not .Indirect}}{{.}}{{end}}' -m all
github.com/pomerium/pomerium
contrib.go.opencensus.io/exporter/jaeger v0.1.0
contrib.go.opencensus.io/exporter/prometheus v0.1.0
github.com/fsnotify/fsnotify v1.4.7
github.com/golang/mock v1.3.1
github.com/golang/protobuf v1.3.2
github.com/google/go-cmp v0.3.1
github.com/mitchellh/hashstructure v1.0.0
github.com/pomerium/go-oidc v2.0.0+incompatible
github.com/prometheus/client_golang v0.9.4
github.com/rs/zerolog v1.14.3
github.com/spf13/viper v1.4.0
go.opencensus.io v0.22.0
golang.org/x/crypto v0.0.0-20190611184440-5c40567a22f8
golang.org/x/net v0.0.0-20190611141213-3f473d35a33a
golang.org/x/oauth2 v0.0.0-20190604053449-0f29369cfe45
golang.org/x/xerrors v0.0.0-20190717185122-a985d3407aa7
google.golang.org/api v0.6.0
google.golang.org/grpc v1.22.2
gopkg.in/square/go-jose.v2 v2.3.1
gopkg.in/yaml.v2 v2.2.2

Did you have any concerns about this package in particular?

Thanks!

See also:

@desimone desimone added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Aug 24, 2019
@desimone
Copy link
Contributor

desimone commented Sep 5, 2019

@yegle ping :)

@desimone desimone added NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Sep 5, 2019
@yegle
Copy link
Contributor Author

yegle commented Sep 6, 2019

Hmm, I think you are right. There's no indication the sarama package is actually part of the Pomerium binary:

$ strings pomerium|grep sarama
(no output)

@yegle yegle closed this as completed Sep 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Projects
None yet
Development

No branches or pull requests

3 participants