[MIR] ujson?

[James Page]

[Availability]
In universe

[Rationale]
New dependency for ceilometer (and aodh)

[Security]
No security history.

[Quality assurance]
py3: OK
unit tests during build: OK

[Dependencies]
All in main

[Standards compliance]
OK; debhelper9, python2&3

[Maintenance]
ubuntu-openstack

[Background information]
High performance json parser; important for in-data-band processing of events for telemetry.

[James Page (james-page)]

Bug subscription added for ubuntu-openstack

[Matthias Klose (doko)]

is this package really maintained upstream? looking at https://github.com/esnme/ultrajson at the open issues and pull requests for segfaults and memory leaks, this isn't much promising. Granted, it's an upstream patch in ceilometer, but is this usage really performance critical?

[James Page (james-page)]

Reading the original inclusion commit:

https://github.com/openstack/requirements/commit/f610e12d67fc093b713c442da2d973da6d71e0ab

being actively maintained is one of the review criteria; I've raised aodh and ceilometer tasks as it would appear that this might not be the case today (things where being actively maintained 2 years ago when ujson was added to global requirements).

Any opinion from the OpenStack telemetry devs? are the performance gains from ujson sufficient to justify using something other than the standard json module?

[gordon chung (chungg)]

when we originally switched to it in gnocchi[1], a former colleague and i were benchmarking its performance but i seemed to have removed my benchmarks when i updated my computer. i would say http://artem.krylysov.com/blog/2015/09/29/benchmark-python-json-libraries/ is an accurate representation. for our use case, we got more than than the 2x-10x improvement.

i would argue we probably don't need ujson in aodh (we merged it recently so i imagine we could just revert that if required).

for ceilometer, we use it for gnocchi publisher and http publisher.
- the gnocchi publisher we arguably don't need it as it's only used in the event workflow (which is less frequent).
- the http publisher, well it's used for everything. so basically it's a matter of do we care to slow down http publisher that in theory isn't broken. i don't use the http publisher so i'm indifferent and i don't know how much of the workflow is serialisation time.

tl;dr if there is a patch to remove ujson from aodh, i'll +A. for ceilometer, i'm indifferent and will +A depending on feedback.

[1] https://github.com/gnocchixyz/gnocchi/commit/e7d8fafa77b0cef20b653020425c15be76e389b5

[OpenStack Infra (hudson-openstack)]

Related fix proposed to branch: master
Review: https://review.openstack.org/528829

[gordon chung (chungg)]

i proposed revert from aodh just to simplify the scope: https://review.openstack.org/#/c/528829

[James Page (james-page)]

I'll propose a review for ceilometer; with the current state of ujson development we need to make an openstack level decision as to whether ujson should be used or not - if the performance gains are worth it, and we can sort out the maintenance concerns then maybe switch all of openstack from anyjson->ujson via oslo_serialization might be considered.

[OpenStack Infra (hudson-openstack)]

Fix proposed to branch: master
Review: https://review.openstack.org/530891

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.openstack.org/528829
Committed: https://git.openstack.org/cgit/openstack/aodh/commit/?id=4f16ed6c34e60e13fbd4ea14dcc511a154298348
Submitter: Zuul
Branch: master

commit 4f16ed6c34e60e13fbd4ea14dcc511a154298348
Author: gordon chung <email address hidden>
Date: Mon Dec 18 21:52:31 2017 +0000

    Revert "Replace jsonutils by ujson"

    This reverts commit 17feef6c66c552f3b747fd24e4dc7ec46a526b4f.

    ujson isn't necessary required and ubuntu packagers raised concerned.
    replace with json

    Change-Id: Ia53fc9226ab7a8348bf134b3b3aaf37927398cd3
    related-bug: #1737989

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.openstack.org/530891
Committed: https://git.openstack.org/cgit/openstack/ceilometer/commit/?id=05050e4b2110d0807257c2a29c4ab6080813f36d
Submitter: Zuul
Branch: master

commit 05050e4b2110d0807257c2a29c4ab6080813f36d
Author: James Page <email address hidden>
Date: Wed Jan 3 10:11:30 2018 +0000

    Replace ujson with json

    ujson has not had any active maintenance for the last 12 months;
    switch to using json module instead.

    Change-Id: I39027b534e94b3f877d881647a7c843183f60f92
    Closes-Bug: 1737989

[OpenStack Infra (hudson-openstack)]

This issue was fixed in the openstack/ceilometer 10.0.0 release.

[Mathieu Trudel-Lapierre (cyphermox)]

So, what is the outcome of this?

[James Page (james-page)]

No longer required - ujson has been dropped from ceilometer and aodh