Thanks for the comments guys!
Re concurrency, see below. Other nits: this is the code copied from hadoop metrics2. The only lines added are removeMetric() method and after it (3 methods total). Other code is unchanged (as we probably don't want to change it, see below also).
RE having one for dynamic and static: understood. Makes sense.
By refactoring I didn't mean to create two registries, but only extract MetricsSource initialization. Ok, let's leave it as is.
Re exceptions: OK, agreed.
increment shouldn't be on the registry. The registry is there to create and hold metrics.
I agree, I haven't added it. It is from hadoop's code, as Ted Yu mentioned. I.e. I have same thoughts about doing exactly this:
We want the registry to be functionally close to what's in org.apache.hadoop.metrics2 so that if they ever add a remove function then we can use that registry wholesale.
Since you've moved the metrics creation out of the source, it's possible that more then one thread could be using it and everything will need to be done using concurrent hash maps.
There's no access to the registry from outside the BaseMetricsSourceImpl class, so I don't see how extracting changes things. In the MetricsRegistry impl that I added you can see that concurrent maps are used to hold metrics. Didn't think that tags will be created a lot, but rather in initialization methods once, so haven't changed to using concurrent map for it.
At the same time I see that access to maps in hadoop code (and hence the one I copied) is synchronized (didn't notice that, sorry). So it looks like we either need to make added methods to be synchronized, or use concurrent collections. If we do want to change minimally existing hadoop code (for the reasons we said above) it makes sense to just make these methods use those sync internal methods when adding metrics and such.
Thanx again for the comments!