search

yaorg / node-measured

A Node metrics library for measuring and reporting application-level metrics, inspired by Coda Hale, Yammer Inc's Dropwizard Metrics Libraries
https://yaorg.github.io/node-measured/
MIT License
517 stars 52 forks source link

Why isn't DimensionAwareMetricsRegistry a subclass of Collection? #70

Closed Qard closed 5 years ago

Qard commented 5 years ago

I notice there's a lot of duplicated logic between DimensionAwareMetricsRegistry in measured-reporting and Collection in measured-core. Is there a reason DimensionAwareMetricsRegistry was not just created as a subclass of Collection?

What about SelfReportingMetricsRegistry? It currently seems to contain a DimensionAwareMetricsRegistry as a field by default, allowing a custom registry to be given, but if a custom registry is given it needs to conform to the same API including the dimension-specific parts. The dimensions concern also seems to leak out into SelfReportingMetricsRegistry so it seems to me like maybe those should just be merged or subclassed?

What do you think? I can put together some PRs to clean that up. I just want to know what your perspective is on it first.

fieldju commented 5 years ago

So this is slightly complicated to answer without context.

This library used to be felixge/node-measured and it was only most of what is in core today. It had no sense of dimensional data or reporters, but had almost all the core metric types that exist today in measured-core

Collection was @felixge's original impl of a registry. This wasn't going to meet Nike's need, because we need dimensional data, so we went through our open source process to get involved with this project.

It turns out that @felixge didn't want to be involved with this project any more and had moved on to bigger and better things. Since it made no since for Nike to take this over and move it to its space, I created a new org called YAORG Yet Another Org, so that when if and when I move on, there can be at least other people with the proper permissions to keep things alive.

You can see some of that conversation here: https://github.com/yaorg/node-measured/issues/33, the rest of it is in my email history.

Anyways to try and answer your original question, I wanted to keep the core lib as close to the original as possible and keep it as small as possible with as few of dependencies as I could. since I had engineering teams within Nike (I am on a core platform team that creates and manages tools and services for other engineers) that demanded super small super light packages and demanded that I make everything composable. Felix also seemed to have had a policy from previous PRs of not wanting breaking changes.

The easiest thing for me to do was just leave the original Collection class's API (as well as the rest of the original package really) as it was (API wise) and create a new package that people could opt in to, which is where all my focus goes. I do not use the collection in core and would love to delete it and just have core be the core metric types.

TL;DR Collection was part of the original measured lib, I didn't want to step on the original authors toes and break the API. I had Engineers insisting that I had small packages for everything that they could compose. I just made a new reporting module for my multi-dimensional needs.

[edit] almost all of this was a port from the internal project I had at Nike that was built off of measured. So when I opened sourced it I did so as new packages rather than re-writing all my code as if it was part of measured from the start. had I started the code from scratch in OSS as part of this project, it would likely look different and there would be less repetition.

Let me know if that helps.

Cheers,

Justin

fieldju commented 5 years ago

If you want to clean things up, that is fine, I am a huge fan of DRY.

For me having the most beautiful code < causing developer friction during updates.

If you think you have changes that make the code better without causing developer friction or making the code harder to read and understand then I would love PRs.

Qard commented 5 years ago

Ah, cool. Thanks for the context. I was aware of Felix owning this previously. I missed what happened in the change over though and what your guiding principles were on arranging things the way you did.

I'll just leave this as they are for now. I just figured there was some opportunity for cleanup, but if you want to consider Collection separate, that's fine.

fieldju commented 5 years ago

I don't mind refactoring to better follow DRY.

The crux of it is that I basically couldn't mutate any of the original code of measured because I was just pulling it into my internal Nike wrapper project as an NPM dep.

So when I ported that code into this project I did so as new modules. Now that it is all 1 code base you could probably get some gains from refactoring things as you have noticed.

fieldju commented 5 years ago

I don't mind refactoring

I really mean "I don't mind the code being refactored", as in I am not territorial or too emotionally attached.