Dependency to DropWizard Metrics Library (was: Percentile implementation)

[Julian Sedding <js...@gmail.com>]

Hi all

OAK-6430[0] introduced a mandatory dependency to
io.dropwizard.metrics:metrics-core to oak-segment-tar.

Before this change, the runtime dependency to this metrics library was
optional (and it still is in oak-core).

Originally, the dependency was introduced in OAK-3654[1] and a facade
was implemented with the following justification: "To avoid having
dependency on Metrics API all over in Oak we can come up with minimal
interfaces which can be used in Oak and then provide an implementation
backed by Metric."

There was no discussion on the Oak list at the time. However, a
similar discussion happened on the Sling list[2]. Basically, bad past
experiences with breaking changes in the dropwizard metrics API led to
the implementation of a facade in order to limit the potentila impact
of future breaking changes. Of course a facade decouples the code from
the dependency and thus allows plugging in a different implementation
should the need arise.

Therefore, I ask the dev team:
(1) Do we want a mandatory runtime dependency
io.dropwizard.metrics:metrics-core?
(2) Should we revisit OAK-6430 and implement the mechanism via the
facade? Probably extending the HistogramStats interface with a method
"#getPercentile(double)".

IMHO we should avoid the mandatory dependency.

Regards
Julian

[0] https://issues.apache.org/jira/browse/OAK-6430
[1] https://issues.apache.org/jira/browse/OAK-3654
[2] http://markmail.org/thread/47fd5psel2wv2y42



On Thu, Jul 6, 2017 at 2:54 PM, Andrei Dulceanu
<an...@gmail.com> wrote:
>> The only problem that I see is the fact that it doesn't provide a way to
>> easily access a desired percentile (only mean and 75th, 95th, 98th, 99th
>> and 999th). Currently we are using 50th percentile, i.e. mean, but in the
>> future that might change.
>>
>
> Please read median instead of mean above. Implementing the change, I
> discovered Histogram#getSnapshot().getValue(double quantile) which is
> exactly what I was looking for.
>
>
>> I will try to make the adjustments and will revisit the percentile
>> implementation once we'll change our use pattern there.
>>
>
> This change is tracked in OAK-6430 [0] and fixed at r1801043.
>
> [0] https://issues.apache.org/jira/browse/OAK-6430
>
> 2017-07-06 14:55 GMT+03:00 Andrei Dulceanu <an...@gmail.com>:
>
>> Hi Chetan,
>>
>>
>>> Instead of commons-math can we use Metric Histogram  (which I also
>>> suggested earlier in the thread).
>>
>>
>> I took another look at the Metric Histogram and I think at the moment it
>> can be used instead of SynchronizedDescriptiveStatistics from
>> commons-math3. The only problem that I see is the fact that it doesn't
>> provide a way to easily access a desired percentile (only mean and 75th,
>> 95th, 98th, 99th and 999th). Currently we are using 50th percentile, i.e.
>> mean, but in the future that might change.
>>
>>
>>> This would avoid downstream Oak
>>> users to include another dependency as Oak is already using Metrics in
>>> other places.
>>>
>>
>> I will try to make the adjustments and will revisit the percentile
>> implementation once we'll change our use pattern there.
>>
>> Regards,
>> Andrei
>>
>> 2017-07-06 14:38 GMT+03:00 Chetan Mehrotra <ch...@gmail.com>:
>>
>>> Instead of commons-math can we use Metric Histogram  (which I also
>>> suggested earlier in the thread). This would avoid downstream Oak
>>> users to include another dependency as Oak is already using Metrics in
>>> other places.
>>>
>>> Can we reconsider this decision?
>>> Chetan Mehrotra
>>>
>>>
>>> On Tue, Jul 4, 2017 at 4:45 PM, Julian Sedding <js...@gmail.com>
>>> wrote:
>>> > Maybe it is not necessary to embed *all* of commons-math3. The bnd
>>> > tool (used by maven-bundle-plugin) can intelligently embed classes
>>> > from specified java packages, but only if they are referenced.
>>> > Depending on how well commons-math3 is modularized, that could allow
>>> > for much less embedded classes. Neil Bartlett wrote a good blog post
>>> > about this feature[0].
>>> >
>>> > Regards
>>> > Julian
>>> >
>>> > [0] http://njbartlett.name/2014/05/26/static-linking.html
>>> >
>>> >
>>> > On Tue, Jul 4, 2017 at 12:20 PM, Andrei Dulceanu
>>> > <an...@gmail.com> wrote:
>>> >> I'll add the dependency.
>>> >>
>>> >> Thanks,
>>> >> Andrei
>>> >>
>>> >> 2017-07-04 13:10 GMT+03:00 Michael Dürig <md...@apache.org>:
>>> >>
>>> >>>
>>> >>>
>>> >>> On 04.07.17 11:15, Francesco Mari wrote:
>>> >>>
>>> >>>> 2017-07-04 10:52 GMT+02:00 Andrei Dulceanu <
>>> andrei.dulceanu@gmail.com>:
>>> >>>>
>>> >>>>> Now my question is this: do we have a simple percentile
>>> implementation in
>>> >>>>> Oak (I didn't find one)?
>>> >>>>>
>>> >>>>
>>> >>>> I'm not aware of a percentile implementation in Oak.
>>> >>>>
>>> >>>> If not, would you recommend writing my own or adapting/extracting an
>>> >>>>> existing one in a utility class?
>>> >>>>>
>>> >>>>
>>> >>>> In the past we copied and pasted source code from other projects in
>>> >>>> Oak. As long as the license allows it and proper attribution is
>>> given,
>>> >>>> it shouldn't be a problem. That said, I'm not a big fan of either
>>> >>>> rewriting an implementation from scratch or copying and pasting
>>> source
>>> >>>> code from other projects. Is exposing a percentile really necessary?
>>> >>>> If yes, how big of a problem is embedding of commons-math3?
>>> >>>>
>>> >>>>
>>> >>> We should avoid copy paste as we might miss important fixes in later
>>> >>> releases. I only did this once for some code where we needed a fix
>>> that
>>> >>> wasn't yet released. It was a hassle.
>>> >>> I would just add a dependency to commons-math3. Its a library
>>> exposing the
>>> >>> functionality we require, so let's use it.
>>> >>>
>>> >>> Michael
>>> >>>
>>>
>>
>>

Re: Dependency to DropWizard Metrics Library (was: Percentile implementation)

[Andrei Dulceanu <an...@gmail.com>]

Hi Julian,

Therefore, I ask the dev team:
> (1) Do we want a mandatory runtime dependency
> io.dropwizard.metrics:metrics-core?
>

Ideally we could push it back to optional, as it previously was, but I
guess it's a bit more difficult.


> (2) Should we revisit OAK-6430 and implement the mechanism via the
> facade? Probably extending the HistogramStats interface with a method
> "#getPercentile(double)".
>

Suppose we add #getPercentile(double) in HistogramStats. This means a base
implementation should be provided in SimpleStats. But doesn't this defeat
the initial purpose for which the metrics library was added/used? I was
looking for a simple percentile implementation and that's why we ended up
using the one from metrics-core.

Using a "dummy" implementation when metrics-core is not available might
have a negative impact on the scheduler in SegmentNodeStore. The scheduler
uses the percentile information for an optimisation related to some wait
time on a semaphore. Inaccurate data could make the optimisation useless
and event affect overall performance. But in order to have accurate data, a
(decent) percentile implementation is needed. Hence the circle is closed.

Regards,
Andrei

2017-07-17 12:46 GMT+03:00 Julian Sedding <js...@gmail.com>:

> Hi all
>
> OAK-6430[0] introduced a mandatory dependency to
> io.dropwizard.metrics:metrics-core to oak-segment-tar.
>
> Before this change, the runtime dependency to this metrics library was
> optional (and it still is in oak-core).
>
> Originally, the dependency was introduced in OAK-3654[1] and a facade
> was implemented with the following justification: "To avoid having
> dependency on Metrics API all over in Oak we can come up with minimal
> interfaces which can be used in Oak and then provide an implementation
> backed by Metric."
>
> There was no discussion on the Oak list at the time. However, a
> similar discussion happened on the Sling list[2]. Basically, bad past
> experiences with breaking changes in the dropwizard metrics API led to
> the implementation of a facade in order to limit the potentila impact
> of future breaking changes. Of course a facade decouples the code from
> the dependency and thus allows plugging in a different implementation
> should the need arise.
>
> Therefore, I ask the dev team:
> (1) Do we want a mandatory runtime dependency
> io.dropwizard.metrics:metrics-core?
> (2) Should we revisit OAK-6430 and implement the mechanism via the
> facade? Probably extending the HistogramStats interface with a method
> "#getPercentile(double)".
>
> IMHO we should avoid the mandatory dependency.
>
> Regards
> Julian
>
> [0] https://issues.apache.org/jira/browse/OAK-6430
> [1] https://issues.apache.org/jira/browse/OAK-3654
> [2] http://markmail.org/thread/47fd5psel2wv2y42
>
>
>
> On Thu, Jul 6, 2017 at 2:54 PM, Andrei Dulceanu
> <an...@gmail.com> wrote:
> >> The only problem that I see is the fact that it doesn't provide a way to
> >> easily access a desired percentile (only mean and 75th, 95th, 98th, 99th
> >> and 999th). Currently we are using 50th percentile, i.e. mean, but in
> the
> >> future that might change.
> >>
> >
> > Please read median instead of mean above. Implementing the change, I
> > discovered Histogram#getSnapshot().getValue(double quantile) which is
> > exactly what I was looking for.
> >
> >
> >> I will try to make the adjustments and will revisit the percentile
> >> implementation once we'll change our use pattern there.
> >>
> >
> > This change is tracked in OAK-6430 [0] and fixed at r1801043.
> >
> > [0] https://issues.apache.org/jira/browse/OAK-6430
> >
> > 2017-07-06 14:55 GMT+03:00 Andrei Dulceanu <an...@gmail.com>:
> >
> >> Hi Chetan,
> >>
> >>
> >>> Instead of commons-math can we use Metric Histogram  (which I also
> >>> suggested earlier in the thread).
> >>
> >>
> >> I took another look at the Metric Histogram and I think at the moment it
> >> can be used instead of SynchronizedDescriptiveStatistics from
> >> commons-math3. The only problem that I see is the fact that it doesn't
> >> provide a way to easily access a desired percentile (only mean and 75th,
> >> 95th, 98th, 99th and 999th). Currently we are using 50th percentile,
> i.e.
> >> mean, but in the future that might change.
> >>
> >>
> >>> This would avoid downstream Oak
> >>> users to include another dependency as Oak is already using Metrics in
> >>> other places.
> >>>
> >>
> >> I will try to make the adjustments and will revisit the percentile
> >> implementation once we'll change our use pattern there.
> >>
> >> Regards,
> >> Andrei
> >>
> >> 2017-07-06 14:38 GMT+03:00 Chetan Mehrotra <ch...@gmail.com>:
> >>
> >>> Instead of commons-math can we use Metric Histogram  (which I also
> >>> suggested earlier in the thread). This would avoid downstream Oak
> >>> users to include another dependency as Oak is already using Metrics in
> >>> other places.
> >>>
> >>> Can we reconsider this decision?
> >>> Chetan Mehrotra
> >>>
> >>>
> >>> On Tue, Jul 4, 2017 at 4:45 PM, Julian Sedding <js...@gmail.com>
> >>> wrote:
> >>> > Maybe it is not necessary to embed *all* of commons-math3. The bnd
> >>> > tool (used by maven-bundle-plugin) can intelligently embed classes
> >>> > from specified java packages, but only if they are referenced.
> >>> > Depending on how well commons-math3 is modularized, that could allow
> >>> > for much less embedded classes. Neil Bartlett wrote a good blog post
> >>> > about this feature[0].
> >>> >
> >>> > Regards
> >>> > Julian
> >>> >
> >>> > [0] http://njbartlett.name/2014/05/26/static-linking.html
> >>> >
> >>> >
> >>> > On Tue, Jul 4, 2017 at 12:20 PM, Andrei Dulceanu
> >>> > <an...@gmail.com> wrote:
> >>> >> I'll add the dependency.
> >>> >>
> >>> >> Thanks,
> >>> >> Andrei
> >>> >>
> >>> >> 2017-07-04 13:10 GMT+03:00 Michael Dürig <md...@apache.org>:
> >>> >>
> >>> >>>
> >>> >>>
> >>> >>> On 04.07.17 11:15, Francesco Mari wrote:
> >>> >>>
> >>> >>>> 2017-07-04 10:52 GMT+02:00 Andrei Dulceanu <
> >>> andrei.dulceanu@gmail.com>:
> >>> >>>>
> >>> >>>>> Now my question is this: do we have a simple percentile
> >>> implementation in
> >>> >>>>> Oak (I didn't find one)?
> >>> >>>>>
> >>> >>>>
> >>> >>>> I'm not aware of a percentile implementation in Oak.
> >>> >>>>
> >>> >>>> If not, would you recommend writing my own or adapting/extracting
> an
> >>> >>>>> existing one in a utility class?
> >>> >>>>>
> >>> >>>>
> >>> >>>> In the past we copied and pasted source code from other projects
> in
> >>> >>>> Oak. As long as the license allows it and proper attribution is
> >>> given,
> >>> >>>> it shouldn't be a problem. That said, I'm not a big fan of either
> >>> >>>> rewriting an implementation from scratch or copying and pasting
> >>> source
> >>> >>>> code from other projects. Is exposing a percentile really
> necessary?
> >>> >>>> If yes, how big of a problem is embedding of commons-math3?
> >>> >>>>
> >>> >>>>
> >>> >>> We should avoid copy paste as we might miss important fixes in
> later
> >>> >>> releases. I only did this once for some code where we needed a fix
> >>> that
> >>> >>> wasn't yet released. It was a hassle.
> >>> >>> I would just add a dependency to commons-math3. Its a library
> >>> exposing the
> >>> >>> functionality we require, so let's use it.
> >>> >>>
> >>> >>> Michael
> >>> >>>
> >>>
> >>
> >>
>