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

synapse.metrics: implement detailed memory usage reporting on PyPy #7536

Merged
merged 3 commits into from
May 22, 2020

Conversation

intelfx
Copy link
Contributor

@intelfx intelfx commented May 19, 2020

PyPy's gc.get_stats() returns an object containing detailed allocator statistics
which could be beneficial to collect as metrics.

Signed-off-by: Ivan Shapovalov intelfx@intelfx.name

Pull Request Checklist

  • Pull request is based on the develop branch
  • Pull request includes a changelog file. The entry should:
    • Be a short description of your change which makes sense to users. "Fixed a bug that prevented receiving messages from other servers." instead of "Moved X method from EventStore to EventWorkerStore.".
    • Use markdown where necessary, mostly for code blocks.
    • End with either a period (.) or an exclamation mark (!).
    • Start with a capital letter.
  • Pull request includes a sign off
  • Code style is correct (run the linters)

@intelfx intelfx force-pushed the pypy-gc-metrics branch from 52a4e85 to b08e1a0 Compare May 19, 2020 21:03
@intelfx intelfx marked this pull request as ready for review May 19, 2020 21:03
@intelfx
Copy link
Contributor Author

intelfx commented May 19, 2020

Should I add sample panes to the contrib Grafana dashboard?

@intelfx intelfx force-pushed the pypy-gc-metrics branch from b08e1a0 to fb816dd Compare May 19, 2020 21:07
@richvdh
Copy link
Member

richvdh commented May 20, 2020

Should I add sample panes to the contrib Grafana dashboard?

I'd rather not, as it will mean that everybody else who uses the dashboard ends up with useless panes. I'm not quite sure of the best way forward here though.

In any case, please can you fix the CI failures here?

@intelfx
Copy link
Contributor Author

intelfx commented May 20, 2020

@richvdh I'm not sure what I should do wrt CI failures.

The style check wants me to reformat this:

        pypy_gc_time = CounterMetricFamily(
            "pypy_gc_time_seconds_total",
            "Total time spent in PyPy GC",
            labels=[],
        )

        <...>

        pypy_mem = GaugeMetricFamily(
            "pypy_memory_bytes",
            "Memory tracked by PyPy allocator",
            labels=["state", "class", "kind"],
        )

Into this:

        pypy_gc_time = CounterMetricFamily(
            "pypy_gc_time_seconds_total", "Total time spent in PyPy GC", labels=[]
        )

        <...>

        pypy_mem = GaugeMetricFamily(
            "pypy_memory_bytes",
            "Memory tracked by PyPy allocator",
            labels=["state", "class", "kind"],
        )

Losing style consistency in the process. As far as I can see, my style is permitted per the style guide. Do I have to strictly obey the linter's suggestion?

Other two build failures complain that PyPy-specific methods and fields don't exist, but I'm fairly sure none of the new code normally gets executed under CPython.

@richvdh
Copy link
Member

richvdh commented May 20, 2020

As far as I can see, my style is permitted per the style guide. Do I have to strictly obey the linter's suggestion?

yes. The style guide mostly says "do what black tells you to do". Just run black on your changes.

you can add # type: ignore to stop mypy complaining about pypy-only interfaces, if you can't find a better solution.

@intelfx
Copy link
Contributor Author

intelfx commented May 20, 2020

yes. The style guide mostly says "do what black tells you to do". Just run black on your changes.

Hmph. I must've been looking at an older version of the style guide. I don't like the inconsistency, but fine.

you can add # type: ignore to stop mypy complaining about pypy-only interfaces, if you can't find a better solution.

I can't, but then again, I didn't write a synapse in python :) Can you? I'd gladly do something more appropriate, if said something exists.

@intelfx intelfx force-pushed the pypy-gc-metrics branch from fb816dd to 91ab8cf Compare May 20, 2020 12:24
intelfx added 3 commits May 20, 2020 15:35
PyPy's gc.get_stats() returns an object containing detailed allocator statistics
which could be beneficial to collect as metrics.

Signed-off-by: Ivan Shapovalov <intelfx@intelfx.name>
Signed-off-by: Ivan Shapovalov <intelfx@intelfx.name>
@intelfx intelfx force-pushed the pypy-gc-metrics branch from 91ab8cf to 055878e Compare May 20, 2020 12:36
@richvdh richvdh requested a review from a team May 20, 2020 12:42
Copy link
Member

@erikjohnston erikjohnston left a comment

Choose a reason for hiding this comment

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

Looks great! Thanks!

@erikjohnston erikjohnston merged commit ac481a7 into matrix-org:develop May 22, 2020
phil-flex pushed a commit to phil-flex/synapse that referenced this pull request Jun 16, 2020
…atrix-org#7536)

PyPy's gc.get_stats() returns an object containing detailed allocator statistics
which could be beneficial to collect as metrics.

Signed-off-by: Ivan Shapovalov <intelfx@intelfx.name>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants