[BEAM-3563] Adding User metric proto to PTransform metrics proto. #4544
Conversation
| metrics.cells.DistributionData.from_runner_api( | ||
| proto.distribution_data)) | ||
| for step, step_metric in step_metrics.items(): | ||
| print step |
| message User { | ||
|
|
||
| // A key for identifying a metric at the most granular level. | ||
| message MetricKey { |
There was a problem hiding this comment.
We should probably be calling this a MetricName to avoid confusion with the MetricKey class in Java/Python that contains a step id.
| metrics.cells.DistributionData.from_runner_api( | ||
| proto.distribution_data)) | ||
| for step, step_metric in step_metrics.items(): | ||
| print step |
There was a problem hiding this comment.
robertwb wrote:
debugging
Uff. Sorry about that. Removed.
| message User { | ||
|
|
||
| // A key for identifying a metric at the most granular level. | ||
| message MetricKey { |
There was a problem hiding this comment.
robertwb wrote:
We should probably be calling this a MetricName to avoid confusion with the MetricKey class in Java/Python that contains a step id.
In that case, I've moved the runner translations to the MetricName class. Let me know what you think. I named it metric_name, because within it there's another name field.
| } | ||
|
|
||
| // Data associated with a counter metric. | ||
| message CounterData { |
There was a problem hiding this comment.
Incidentally, these make sense at the top level and I have a slight preference for keeping as many things toplevel as possible unless these is a compelling reason to do otherwise. There is usually no downside.
Here it is a particular readability problem:
Useris very misleading if not fully qualified, versus e.g.MetricData- great distance between the start of the message and the fields
There was a problem hiding this comment.
(and is MetricData the same as the existing MetricUpdate? Luke and I chatted and I came away unsure)
There was a problem hiding this comment.
Moving stuff to not be nested is an independent change to what was asked. @kennknowles, if you would like to take on that change, feel free to do so.
There was a problem hiding this comment.
I might, depending on how much this proto enters my life. But we should fix the misleading name either way.
There was a problem hiding this comment.
BTW I don't know of a public discussion where I could gain context on this change. Can you link to that and/or public design doc on user-defined metrics in portability?
Anyhow I'm just providing drive-by feedback on the protos since I tried to use them and it isn't really clear why they are the way they are. I'm sure I can power through and make it work, but it isn't great for people not in our office.
|
|
||
| def to_runner_api(self): | ||
| return beam_fn_api_pb2.Metrics.User.DistributionData( | ||
| return beam_fn_api_pb2.Metrics.PTransform.User.DistributionData( |
There was a problem hiding this comment.
I'm just the peanut gallery, of course, but this doesn't seem clearer at all than beam_fn_api_pb2.DistributionData(...)
robertwb
left a comment
robertwb
left a comment
There was a problem hiding this comment.
LGTM. Could you squash your commits?
| [beam_fn_api_pb2.Metrics.User( | ||
| key=beam_fn_api_pb2.Metrics.User.MetricKey( | ||
| step=self.step_name, namespace=k.namespace, name=k.name), | ||
| metric_name=beam_fn_api_pb2.Metrics.User.MetricName( |
There was a problem hiding this comment.
Perhaps use k.to_runner_api() here?
pabloem
force-pushed
the
change-fn-metrics
branch
from
a1e06aa to
2de70ff
Compare
|
Thanks Robert. Done. |
|
Retest this please. |
|
Ready to merge : ) |
|
Known flake in gradle build. I don't think we should hold up the proto changes for that. |
5 participants
Let me know if this looks good!
r: @robertwb
cc: @lukecwik @kennknowles