Yes totally agreed with you. For me, getValueAndReset() itself is more painful to modify anything regarding metrics. I feel that getValueAndReset() was born with precondition that caller should ensure thread-safe. At least kinds of implementation of IMetric what Storm provides now are NOT thread-safe, and guaranteeing thread-safety is not a requirement for custom IMetric. (Nowhere claims that) Since metrics are being handled per task, and normally reader and writer for IMetric are using same thread so there's no issue at all. But such precondition and backward compatibility makes me giving up collecting metrics at worker level for optimization. (I also saw the complicated logic in CountStatAndMetric and LatencyStatAndMetric which are for handling both heartbeat message and built-in metrics.)
Btw, what I'm concerning is all about backward compatibility. As you may know, what I've been addressing is done without breaking backward compatibility. It's based on precondition: work for Storm 2.0.0 is in progress and in phase 2 we are planning to evaluate metrics feature of JStorm and may adopt. We all don't know when we complete work for Storm 2.0.0 for now and I need some improvements on current metrics so I'm addressing some for 1.x, but if we want to start discussion for new metrics right now, that would be also good. 2016년 5월 18일 (수) 오후 11:22, Bobby Evans <ev...@yahoo-inc.com.invalid>님이 작성: > There are a lot of things that I dislike about IMetric. It provides too > much flexibility and at the same time not enough information/conventions to > be able to interpret the numbers it returns correctly. We recently had a > case where someone was trying to compute an average using a ReducedMetric > and a MeanReducer (which by the way should be deprecated because it is > fundamentally flawed). This hands the metric collector an average. How is > it supposed to combine one average with another when doing a roll up, > either across components or across time ranges? It just does not work > mathematically unless you know that all of the averages had the exact same > number of operations in them, which we cannot know. > This is why dropwizard and other metrics systems have a specific set up > supported metrics, not object, that they know mathematically work out. A > gauge is different from a counter, which is different from a ratio, or a > meter, or a timer, or a histogram. Please lets not reinvent the wheel > here, we already did it wrong once, lets not do it wrong again. We are > using dropwizard in other places in the code internally, I would prefer > that we standardize on it, or a thin wrapper around it based on the same > concepts. Or if there is a different API that someone here would prefer > that we use that is fine with me too. But lets not write it ourselves, > lets take from the experts who have spent a long time building something > that works. > - Bobby > > On Tuesday, May 17, 2016 10:10 PM, Jungtaek Lim <kabh...@gmail.com> > wrote: > > > Hi devs, > > Since IMetric#getValueAndReset doesn't restrict return type, it gives us > flexibility but metrics consumer should parse the value without context > (having just some assumptions). > > I've look into some open source metrics consumers, and many of them support > Number, Map<String, Number/String>, and one of them supports Nested Map. > For the case of Map its key is appended to metric key and value is > converted to 'double'. I think it would be enough, but I'm not sure we can > rely on all metrics consumers to handle properly, too. > > I feel if we can recommend proper types of DataPoint value for storing > metrics to time-series DB via metrics consumer it would be great. It can be > used to protocol between IMetric users and metrics consumer developers. > > What do you think? > > Thanks, > Jungtaek Lim (HeartSaVioR) > > ps. I'm not heavy user of time-series DBs (I researched some, but they > don't document type/size of value clearly) so if someone could give the > information of supporting type/size of value of time-series DBs it should > be great for me. Or we can just assume number as 'double' as above and go > forward. > > >
