Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

support prometheus_client >= 0.4 #4221

Closed
richvdh opened this issue Nov 23, 2018 · 12 comments
Closed

support prometheus_client >= 0.4 #4221

richvdh opened this issue Nov 23, 2018 · 12 comments
Assignees
Labels
A-Logging Synapse's logs (structured or otherwise). Not metrics. z-p2 (Deprecated Label)

Comments

@richvdh
Copy link
Member

richvdh commented Nov 23, 2018

As a quick fix to #4001 / prometheus/client_python#317, we pinned our prometheus client to < 0.4.0.

This introduces problems:

  • Sooner or later we're going to want to pick up some bugfix or feature from more recent prometheus_clients.
  • Arch Linux ships with prometheus_client 0.4, so people on Arch have trouble building synapse (test_metrics fails on both python 2.7 and 3.7 #4167). This problem is likely to affect more platforms in the future.
  • It's actually possible for people to suffer prometheus_client 0.4.0 breaks lots of our metrics #4001 in reverse:
    • They start with synapse 0.33.5 or earlier, which doesn't include the pin, so they get prometheus-client 0.4.0, with the *_total metrics.
    • Then they upgrade synapse, which includes the pin, so prometheus-client gets downgraded.

I'm still surprised at the prometheus_client devs making the choice to rename these metrics, but I think we're going to have to live with it, so could do with a migration plan.

When we have renamed metrics in the past, we have supported both old and new names for a few releases. That gives people a chance to update their dashboards and alerts. I believe we should do something similar here.

It may turn out that supporting both names is more easily done with prometheus_client 0.3 than it is with 0.4, so we should do this work before we are forced to update to 0.4 in a hurry which would break everyone's metrics.

@neilisfragile neilisfragile added z-p2 (Deprecated Label) A-Logging Synapse's logs (structured or otherwise). Not metrics. labels Nov 30, 2018
@matoro
Copy link

matoro commented Dec 23, 2018

I see NixOS having issues with this, Gentoo also dropped 0.3.1 from the repo about a week ago.

@nado
Copy link

nado commented Jan 7, 2019

If we start fresh, is synapse compatible with >0.4.0 ?

@richvdh
Copy link
Member Author

richvdh commented Jan 7, 2019

If we start fresh, is synapse compatible with >0.4.0 ?

Not entirely. The tests fail: #4167

@nado
Copy link

nado commented Jan 7, 2019

Sad, I think this is my last hardblocker for installing it on my system (gentoo). I don’t really intend on porting <prometheus-client-0.4.0 :/

@matoro
Copy link

matoro commented Jan 7, 2019

@nado , I have been running with the following as dev-python/prometheus_client-0.3.1.ebuild:

# Copyright 1999-2018 Gentoo Foundation
# Distributed under the terms of the GNU General Public License v2

EAPI=6

PYTHON_COMPAT=( python{2_7,3_4,3_5,3_6} )
inherit distutils-r1

DESCRIPTION="Python client for the Prometheus monitoring system"
HOMEPAGE="https://pypi.org/project/prometheus_client/"
SRC_URI="https://github.com/prometheus/client_python/archive/v${PV}.tar.gz -> ${P}.tar.gz"

LICENSE="Apache-2.0"
SLOT="0"
KEYWORDS="~amd64"
IUSE="test"

S=${WORKDIR}/client_python-${PV}

RDEPEND="|| ( dev-python/twisted[${PYTHON_USEDEP}] dev-python/twisted-web[${PYTHON_USEDEP}] )"
DEPEND="dev-python/setuptools[${PYTHON_USEDEP}]
        test? ( ${RDEPEND}
                dev-python/pytest[${PYTHON_USEDEP}] )"

python_test() {
        pytest || die
}

which seems to work, but then again it shouldn't really matter if you don't use the metrics functionality.

@nado
Copy link

nado commented Jan 7, 2019

Yeah I could, but since I intended to make a PR against gentoo tree as a long goal, it seems weird to add back a removed ebuild. It might be okay.

But there’s one thing I don't understand, is it a required or an optional dependency?
https://github.com/matrix-org/synapse/blob/master/synapse/python_dependencies.py#L64 leads me to believe it is required, but your comment makes me thinking otherwise.

@matoro
Copy link

matoro commented Jan 7, 2019

It is required as specified by the build system. I just mean that if you don't use those features, it will never be used at runtime. IMO an ebuild should reflect what upstream says, and only make functionality optional at such time upstream is willing to guarantee that it is written sufficiently modularly.

I have also been running synapse quite well on both x86_64 and armv7a with this ebuild coupled with the gerislay overlay. This discussion is probably getting a bit off-topic now so if there is another more Gentoo-focused location (issue on an overlay repo?) we should probably use that for further work.

@richvdh
Copy link
Member Author

richvdh commented Jan 8, 2019

If we start fresh, is synapse compatible with >0.4.0 ?

Not entirely. The tests fail: #4167

Actually it sounds like this has been fixed by #4317

@matoro
Copy link

matoro commented Jan 10, 2019

FreeBSD is getting hit by this as well: https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=234828

@xavierba
Copy link

Fedora is being hit too.
From what I read above, the test failure has been fixed. Is anyone working on the (possibly big) remaining stuff ? Should that be marked as a blocker for 1.0 release ?

@cyphar
Copy link

cyphar commented Jun 10, 2019

All openSUSE releases now have this problem. If #4317 fixed the remaining problems with post-0.4 prometheus_client then maybe we should update python_dependencies.py so that folks don't hit errors on start with new distributions.

Heck, I'd be happy to try writing patches to fix any remaining issues, if it's necessary. Otherwise we won't be able to provide 1.0 synapse on openSUSE.

@hawkowl
Copy link
Contributor

hawkowl commented Jul 18, 2019

Fixed in #5636

@hawkowl hawkowl closed this as completed Jul 18, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A-Logging Synapse's logs (structured or otherwise). Not metrics. z-p2 (Deprecated Label)
Projects
None yet
Development

No branches or pull requests

7 participants