search

prometheus / common

Go libraries shared across Prometheus components and libraries.
Apache License 2.0
259 stars 306 forks source link

feat(dependency): add diff package to common #648

Closed dongjiang1989 closed 1 week ago

dongjiang1989 commented 3 weeks ago

Add diff package to common for client-golang, prometheus and alertmanager.

issue: https://github.com/prometheus/client_golang/issues/1490 pr for client-glang: https://github.com/prometheus/client_golang/pull/1499

SuperQ commented 1 week ago

Wait, no, we should not be copy-pasting code like this.

Fixing an EoL package by replacing it with something not EoL is just fine. There's no need to fully remove a dependency.

CC @kakkoyun

kakkoyun commented 1 week ago

Wait, no, we should not be copy-pasting code like this.

Fixing an EoL package by replacing it with something not EoL is just fine. There's no need to fully remove a dependency.

CC @kakkoyun

We did this in client_golang to reduce the external dependencies. And it is small enough and only used int the tests. I never thought it would be needed elsewhere.

That being said, I'd be more than happy to accept https://github.com/prometheus/client_golang/pull/1539.

SuperQ commented 1 week ago

That was incorrectly done in client_golang. The request was to replace the dependency that was abandoned, not reduce it.

kakkoyun commented 1 week ago

That was incorrectly done in client_golang. The request was to replace the dependency that was abandoned, not reduce it.

I'm well aware of the request. We wanted to kill two birds with one stone.

ArthurSens commented 1 week ago

Add diff package to common for client-golang, prometheus and alertmanager.

I'm not sure if I'm missing something, but does prometheus and alertmanager really use this same logic? I'm aware that client_golang uses this for testing, but maybe we could avoid adding it to common if it's only for client_golang.

Now that https://github.com/prometheus/client_golang/pull/1539 was accepted it's even less important to have this here from my point of view 🤔

SuperQ commented 1 week ago

IMO, we don't want to get into the habit of in-lining Go dependencies. This sets a hard to deal with policy long-term. What is the limit for an inline? How do we keep inline code up-to-date?