Agree with all of this. It echoes a thread on the doc that I was going to bring here. Let's keep it simple and use concrete use cases to drive additional abstraction if/when it becomes compelling.
Kenn On Thu, Apr 12, 2018 at 9:21 AM Ben Chambers <bjchamb...@gmail.com> wrote: > Sounds perfect. Just wanted to make sure that "custom metrics of supported > type" didn't include new ways of aggregating ints. As long as that means we > have a fixed set of aggregations (that align with what what users want and > metrics back end support) it seems like we are doing user metrics right. > > - Ben > > On Wed, Apr 11, 2018, 11:30 PM Romain Manni-Bucau <rmannibu...@gmail.com> > wrote: > >> Maybe leave it out until proven it is needed. ATM counters are used a lot >> but others are less mainstream so being too fine from the start can just >> add complexity and bugs in impls IMHO. >> >> Le 12 avr. 2018 08:06, "Robert Bradshaw" <rober...@google.com> a écrit : >> >>> By "type" of metric, I mean both the data types (including their >>> encoding) and accumulator strategy. So sumint would be a type, as would >>> double-distribution. >>> >>> On Wed, Apr 11, 2018 at 10:39 PM Ben Chambers <bjchamb...@gmail.com> >>> wrote: >>> >>>> When you say type do you mean accumulator type, result type, or >>>> accumulator strategy? Specifically, what is the "type" of sumint, sumlong, >>>> meanlong, etc? >>>> >>>> On Wed, Apr 11, 2018, 9:38 PM Robert Bradshaw <rober...@google.com> >>>> wrote: >>>> >>>>> Fully custom metric types is the "more speculative and difficult" >>>>> feature that I was proposing we kick down the road (and may never get to). >>>>> What I'm suggesting is that we support custom metrics of standard type. >>>>> >>>>> On Wed, Apr 11, 2018 at 5:52 PM Ben Chambers <bchamb...@apache.org> >>>>> wrote: >>>>> >>>>>> The metric api is designed to prevent user defined metric types based >>>>>> on the fact they just weren't used enough to justify support. >>>>>> >>>>>> Is there a reason we are bringing that complexity back? Shouldn't we >>>>>> just need the ability for the standard set plus any special system >>>>>> metrivs? >>>>>> On Wed, Apr 11, 2018, 5:43 PM Robert Bradshaw <rober...@google.com> >>>>>> wrote: >>>>>> >>>>>>> Thanks. I think this has simplified things. >>>>>>> >>>>>>> One thing that has occurred to me is that we're conflating the idea >>>>>>> of custom metrics and custom metric types. I would propose the >>>>>>> MetricSpec >>>>>>> field be augmented with an additional field "type" which is a urn >>>>>>> specifying the type of metric it is (i.e. the contents of its payload, >>>>>>> as >>>>>>> well as the form of aggregation). Summing or maxing over ints would be a >>>>>>> typical example. Though we could pursue making this opaque to the >>>>>>> runner in >>>>>>> the long run, that's a more speculative (and difficult) feature to >>>>>>> tackle. >>>>>>> This would allow the runner to at least aggregate and report/return to >>>>>>> the >>>>>>> SDK metrics that it did not itself understand the semantic meaning of. >>>>>>> (It >>>>>>> would probably simplify much of the specialization in the runner itself >>>>>>> for >>>>>>> metrics that it *did* understand as well.) >>>>>>> >>>>>>> In addition, rather than having UserMetricOfTypeX for every type X >>>>>>> one would have a single URN for UserMetric and it spec would designate >>>>>>> the >>>>>>> type and payload designate the (qualified) name. >>>>>>> >>>>>>> - Robert >>>>>>> >>>>>>> >>>>>>> >>>>>>> On Wed, Apr 11, 2018 at 5:12 PM Alex Amato <ajam...@google.com> >>>>>>> wrote: >>>>>>> >>>>>>>> Thank you everyone for your feedback so far. >>>>>>>> I have made a revision today which is to make all metrics refer to >>>>>>>> a primary entity, so I have restructured some of the protos a little >>>>>>>> bit. >>>>>>>> >>>>>>>> The point of this change was to futureproof the possibility of >>>>>>>> allowing custom user metrics, with custom aggregation functions for its >>>>>>>> metric updates. >>>>>>>> Now that each metric has an aggregation_entity associated with it >>>>>>>> (e.g. PCollection, PTransform), we can design an approach which >>>>>>>> forwards >>>>>>>> the opaque bytes metric updates, without deserializing them. These are >>>>>>>> forwarded to user provided code which then would deserialize the metric >>>>>>>> update payloads and perform the custom aggregations. >>>>>>>> >>>>>>>> I think it has also simplified some of the URN metric protos, as >>>>>>>> they do not need to keep track of ptransform names inside themselves >>>>>>>> now. >>>>>>>> The result is simpler structures, for the metrics as the entities are >>>>>>>> pulled outside of the metric. >>>>>>>> >>>>>>>> I have mentioned this in the doc now, and wanted to draw attention >>>>>>>> to this particular revision. >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> On Tue, Apr 10, 2018 at 9:53 AM Alex Amato <ajam...@google.com> >>>>>>>> wrote: >>>>>>>> >>>>>>>>> I've gathered a lot of feedback so far and want to make a decision >>>>>>>>> by Friday, and begin working on related PRs next week. >>>>>>>>> >>>>>>>>> Please make sure that you provide your feedback before then and I >>>>>>>>> will post the final decisions made to this thread Friday afternoon. >>>>>>>>> >>>>>>>>> >>>>>>>>> On Thu, Apr 5, 2018 at 1:38 AM Ismaël Mejía <ieme...@gmail.com> >>>>>>>>> wrote: >>>>>>>>> >>>>>>>>>> Nice, I created a short link so people can refer to it easily in >>>>>>>>>> future discussions, website, etc. >>>>>>>>>> >>>>>>>>>> https://s.apache.org/beam-fn-api-metrics >>>>>>>>>> >>>>>>>>>> Thanks for sharing. >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> On Wed, Apr 4, 2018 at 11:28 PM, Robert Bradshaw < >>>>>>>>>> rober...@google.com> wrote: >>>>>>>>>> > Thanks for the nice writeup. I added some comments. >>>>>>>>>> > >>>>>>>>>> > On Wed, Apr 4, 2018 at 1:53 PM Alex Amato <ajam...@google.com> >>>>>>>>>> wrote: >>>>>>>>>> >> >>>>>>>>>> >> Hello beam community, >>>>>>>>>> >> >>>>>>>>>> >> Thank you everyone for your initial feedback on this proposal >>>>>>>>>> so far. I >>>>>>>>>> >> have made some revisions based on the feedback. There were >>>>>>>>>> some larger >>>>>>>>>> >> questions asking about alternatives. For each of these I have >>>>>>>>>> added a >>>>>>>>>> >> section tagged with [Alternatives] and discussed my >>>>>>>>>> recommendation as well >>>>>>>>>> >> as as few other choices we considered. >>>>>>>>>> >> >>>>>>>>>> >> I would appreciate more feedback on the revised proposal. >>>>>>>>>> Please take >>>>>>>>>> >> another look and let me know >>>>>>>>>> >> >>>>>>>>>> >> >>>>>>>>>> https://docs.google.com/document/d/1MtBZYV7NAcfbwyy9Op8STeFNBxtljxgy69FkHMvhTMA/edit >>>>>>>>>> >> >>>>>>>>>> >> Etienne, I would appreciate it if you could please take >>>>>>>>>> another look after >>>>>>>>>> >> the revisions I have made as well. >>>>>>>>>> >> >>>>>>>>>> >> Thanks again, >>>>>>>>>> >> Alex >>>>>>>>>> >> >>>>>>>>>> > >>>>>>>>>> >>>>>>>>>