Metrics follow-up thoughts

[Cantor, Scott]

On 9/16/16, 3:47 PM, "dev on behalf of Brent Putman" <dev-bounces at shibboleth.net on behalf of putmanb at georgetown.edu> wrote:

> Metrics itself seems pretty good (not that I'm terribly familiar with
> this space).  It's pretty simple and the docs are pretty clear.

I think the whole metrics-core code base is fairly small and mostly taken up by the built-in Reporters and the rest is just a bunch of map maintenance code.
    
> I'm definitely not saying we have to do anything like that.  This
> approach achieves non-invasiveness at the cost of real technological
> sophistication and complexity, which probably requires a great deal
> more expert knowledge than merely adding Metrics stuff directly to a class.

I'll take a look.

> I was thinking about something conceptually similar to Loggers and
> LoggerFactory, where instead of interacting with the Metrics classes
> directly, you did so through a support class (like LoggerFactory),
> which sort of mirrored the important methods of MetricRegistry.  If
> metrics was turned off, it would return objects that just did no-ops,
> etc.  Or something like that.

I hadn't thought of that. The two could probably be combined, meaning a simple implementation of the code to return the objects that might be no-op'd might be based on use of logging categories, just to save the work of reimplementing something exactly like it.
    
> That granularity and hierarchical config does seem nice to have, if it
> can be made to work.

The naming's entirely up to us so there's no reason we can't do it. I don't think that the metrics necessarily correspond to class names like the loggers do, particularly since so many are impl classes, but the same general package names should apply.

Conceptually, I think the goal should be that measuring things on INFO should be production-friendly, just like logging is. Stuff that really should be tracked in 99% of cases would be WARN level. Etc.

> Hmm, yeah, good point.  I thought they mentioned stuff about being
>    thread-safe everywhere, but maybe it doesn't work if you are literally
>    trying to register an existing name.At least the ones like
>  counter(...) though use a getOrAdd approach, so I think those might
>  actually work within the prototypes.  Maybe for the register(...), e.g.
>  of a Guage, they have a way to get an existing one rather than create a
>  new one.  They do have Maps of all these things on the registry.
    
Are they documented that way, or did you just look at the counter() code? I didn't see any sign that's a contract they're making.

The creation of the metrics is definitely something I'm still not clear on, but the idea of fully separating that from the code using them seems like a good idea, if we combine that with the idea of building a simple registry layer to get at them. Then there's no creating them in the code, they're either there or they're no-op'd.

If something in particular is expensive to even gather and push into a metric, we could define an interface (DisabledMetric) to implement on the no-op cases so that we'd have the ability to guard that in the classes if we needed it. That's what I envisioned with the logging idea, but that could be abstracted away.

It would be nice to reduce the invasiveness of the Metrics API to just the actual Metric classes and subclasses, since those are ultimately pretty much the primitives anyway.

Maybe it's enough to just subclass them into our own packages, and then if we ever had to get off it, we just have to implement the base classes we were inheriting, but all the signatures of the objects are in our packages.

-- Scott