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 >>> >> >>> > >>> >>
