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

update opencensus packages #70

Merged
merged 2 commits into from
Nov 6, 2019
Merged

Conversation

mightyguava
Copy link
Contributor

@mightyguava mightyguava commented Aug 5, 2019

The prometheus and datadog exporters were moved into contrib.go.opencensus.io (this whole repo seems unstable and experimental btw).

I'm updating these because I'm vendoring ld-relay into another repo, to use as a library

@eli-darkly
Copy link
Contributor

This is currently only passing CI in Go 1.10.

@mightyguava
Copy link
Contributor Author

Depending on opencensus probably wasn't the wisest choice:
image

@mightyguava
Copy link
Contributor Author

I pinned the version fo go.opencensus.io, hopefully that fixes things? 🙏

@mightyguava
Copy link
Contributor Author

@eli-darkly we are green!

@eli-darkly
Copy link
Contributor

@mightyguava Well, since the ld-relay build uses dep, pinning the version does work. That won't help anyone who pulls this code into an app without using dep. But it's probably reasonable to require that they use it if they do that.

@mightyguava
Copy link
Contributor Author

mightyguava commented Aug 5, 2019

I think it's reasonable to expect people pulling it ld-relay as a library (I might be the only one), to use either dep or go mod.

@alecthomas
Copy link

Any chance we can get this merged?

@mightyguava
Copy link
Contributor Author

I'm one of the people who don't use dep and ran into build breakages. This fixes things for people who use dep. I don't really understand why there's hesitance to merging this pr.

@alecthomas We are using my fork at https://github.com/mightyguava/ld-relay internally. Feel free to make PRs there for whatever you want to change.

@eli-darkly
Copy link
Contributor

@mightyguava There wasn't any deliberate "hesitance"; we just had a lot of other things going on with ld-relay, and had not had a chance to look at this again since the build got fixed. We will take another look. Sorry for the delay.

@eli-darkly eli-darkly changed the base branch from v5 to contrib November 4, 2019 18:59
@eli-darkly
Copy link
Contributor

[tracked internally as 55052]

Copy link
Contributor

@eli-darkly eli-darkly left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks correct; I'll merge it now into onto a branch where we can do some follow-up testing, and assuming there are no unforeseen problems we'll release it ASAP.

@eli-darkly
Copy link
Contributor

And sorry again for the extended delay.

@eli-darkly eli-darkly merged commit 592fd0d into launchdarkly:contrib Nov 6, 2019
LaunchDarklyCI pushed a commit that referenced this pull request Nov 7, 2019
@eli-darkly
Copy link
Contributor

@mightyguava Problem: the Prometheus integration appears to be broken now, due to a transitive dependency on a buggy version of github.com/prometheus/client_golang. To fix this, we'll need to either make dep override the transitive dependency, or wait until we can get that package patched.

Unfortunately, we released 5.8.2 with these changes before we noticed the problem. So we do need to put out another patch ASAP, and I'm not sure if the best way to go at this point is to use an override as I mentioned, or just roll back the dependency change for now. But I'm unclear on what kind of build-breakage you were seeing before, that made this necessary; I should have asked that in the first place.

@mightyguava
Copy link
Contributor Author

Hey, sorry for the breakage. Feel free to roll this back since we do keep our own fork.

The reason we needed to bump this is because when this repo is used with go modules, it automatically pulls in a newer version of opencensus that has breaking API changes, and we wanted to keep the code in sync. The fork basically just maintains the go module manifests. We had to lock the version in this upstream repo though, because without it, upgrading pulled in a version of opencensus that was really broken.

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 this pull request may close these issues.

3 participants