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

Remove common/log. #218

Closed
wants to merge 1 commit into from
Closed

Remove common/log. #218

wants to merge 1 commit into from

Conversation

brian-brazil
Copy link
Contributor

This is no longer used by the Prometheus org. We've switched to promlog
and go-kit logging.

Signed-off-by: Brian Brazil brian.brazil@robustperception.io

@SuperQ
Copy link
Member

SuperQ commented Jan 15, 2020

I think we should mark it Deprecated before we delete it.

So, something like:

  • Add Deprecated: to the package.
  • Release 0.7.0.
  • Delete the code.
  • Release 1.0.0.

@brian-brazil
Copy link
Contributor Author

Why should we do that? It's internal-only code we no longer use, we haven't ever done that for any other bit of of internal code we've removed.

I also have no plans to ever release a 1.0 for this repo.

@SuperQ
Copy link
Member

SuperQ commented Jan 15, 2020

"Internal" is relative in golang vendoring. There are still other exporters outside of gitlab.com/prometheus that use this code. For example, I still need to remove it from the bind_exporter.

Even if we don't release 1.0, marking it deprecated for at least one 0.x is the right thing to do.

@brian-brazil
Copy link
Contributor Author

brian-brazil commented Jan 15, 2020

"Internal" is relative in golang vendoring.

I don't see how that's relevant here, internal is internal. You can't end up depending on this code without having added an explicit dependency on some internal part of a Promethes repo (client_golang is our only public Go API, and doesn't depend on this code).

bind_exporter directly depends on this for example. It's also using both modules and vendoring, so builds will continue to work.

marking it deprecated for at least one 0.x is the right thing to do.

I reiterate that we should not be going through deprecation cycles for purely internal code, that would be quite constraining. If you depend on our internal code that's entirely at your own risk, and that includes the code being deleted out from under you without warning. In this case there has been a few months warning given.

@beorn7
Copy link
Member

beorn7 commented Jan 20, 2020

"Legally", we are allowed to do all kind of breaking changes in common because it is still pre-v1.

In practice, I think we can try to be nice if it is not a huge effort from our side, but we should not let breakage concerns stand in the way of progress.

The communication about the stability guarantees and the "we assume it as internal" part could be better.

Also, while the log package is marked as deprecated in the README.md, it was never marked as deprecated in the "Go way", i.e. https://github.com/golang/go/wiki/Deprecated .

How about we do that now, then create a new version tag so that people can see the warnings in their linters for a while, and then delete the package after a few weeks?

Starting to work with v1+ is another story that we might consider. But given the many moving parts here, we would probably go up with major release numbers quite quickly. (Not necessarily a bad thing, but we should decide about that in an informed way.)

@brian-brazil
Copy link
Contributor Author

How about we do that now, then create a new version tag so that people can see the warnings in their linters for a while, and then delete the package after a few weeks?

I don't like delaying progress due to uses that are explicitly not supported - that's how ossification happens. I especially don't like it when it's being proposed to delay things at the last minute, rather than back in September when the intention to delete this was announced.

This is no longer used by the Prometheus org. We've switched to promlog
and go-kit logging.

Signed-off-by: Brian Brazil <brian.brazil@robustperception.io>
@SuperQ
Copy link
Member

SuperQ commented Jan 20, 2020

Sadly, we simply forgot to use the Go deprecation when we updated the package last September. Mistakes happen, we're not trying to block progress here, just do a couple things in the right order.

@brian-brazil
Copy link
Contributor Author

As I've said we've never gone through a deprecation cycle for our own internal code before, why should we start now?

@beorn7
Copy link
Member

beorn7 commented Jan 20, 2020

As I've said we've never gone through a deprecation cycle for our own internal code before, why should we start now?

  1. The cost is minimal (PR for proper deprecation is out, then we only have to tag and wait for a few weeks).
  2. It isn't well advertised that this code is internal. So we will have external users. Sure, it's their own fault. But why shouldn't we be nice if it costs us so little?

@brian-brazil
Copy link
Contributor Author

The cost is minimal

This is true in this case. I'm very wary of getting into a situation where by trying to be nice we gradually end up with a defacto policy that slows down or prevents development. This is why I take such a strong standpoint on this, as I can see the bad place where doing otherwise will likely land us.

From my standpoint I've already made efforts to be nice. I had done of spot check of who was depending on this to give an idea of impact (which is pretty close to none, almost everything was abandoned), and part of the reason for my email to -developers was to both provide notice and communicate the internal nature of this repo for those not already aware.

It isn't well advertised that this code is internal.

By default all code (in this org or otherwise) is internal, unless there's an indication otherwise. I don't believe we're ever advertised that it is for general consumption, other than for expfmt.

@beorn7
Copy link
Member

beorn7 commented Jan 20, 2020

It isn't well advertised that this code is internal.

By default all code (in this org or otherwise) is internal, unless there's an indication otherwise. I don't believe we're ever advertised that it is for general consumption, other than for expfmt.

My statement is not in contradiction with your statement.

@beorn7
Copy link
Member

beorn7 commented Jan 20, 2020

I don't buy the "slippery slope" argument. I'll always be nicer if it has negligible cost. If somebody assumes that means I'll also be nicer if it has a significant cost, it's their loss, not mine.

In this case, I suggest to tag another release soon (or now), and then merge this PR in a couple of weeks time. (For Go Module users, we could merge it immediately, but not all use Go Modules.)

@SuperQ
Copy link
Member

SuperQ commented Jan 20, 2020

I agree, this isn't likely to generate a huge bureaucratic nightmare. It's just a small amount of code housekeeping.

@beorn7
Copy link
Member

beorn7 commented Jan 20, 2020

I have tagged v0.9.1 (just bug-fix release because it was a "bug" that we didn't mark the deprecation in a way the linters would see).

@roidelapluie
Copy link
Member

I think we can now move forward with this.

0.10.0 was released mid-may.

Base automatically changed from master to main February 23, 2021 19:38
Base automatically changed from main to master February 23, 2021 19:42
Base automatically changed from master to main February 26, 2021 16:07
@bboreham
Copy link
Member

Superceded by #306 ?

@brian-brazil
Copy link
Contributor Author

Yip.

alanprot pushed a commit to alanprot/common that referenced this pull request Mar 15, 2023
…env-with-options

tracing: add options to NewFromEnv
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.

None yet

5 participants