On Tue, Aug 21, 2018 at 2:05 AM Alex Amato <[email protected]> wrote: > > I discovered something while trying to update test_progress_metrics in > fn_api_runner_tests.py to inspect the returned MonitoringInfos in addition to > the already returned metrics format. > > This metric appears to be added twice using the None tag (but overwrites a > previous one). I am not sure if its intentional or not. Please let me know if > this is intentionally overwriting what is supposed to be the same metric, or > if something might be wrong here. > > See the use of element count metrics: > > Here the metric is added using the self.tagged_receivers tag sin the > DoOperation to add the element count metric. This can be the value 'None' > Here the ONLY_OUTPUT tag is used and overridden later. > > Then fix_output_tags in bundle_processor.by assigns the tag, which in this > case is None again
There is a TODO about plumbing the names to the right place that could avoid the use of ONLY_OUTPUT altogether. (Given that all but one operation has multiple outputs, maybe that's not worth it though.) > When the second instance of the metric is added it gets overwritten in the > output_element_counts (because it uses the same key). Is it intentional to > overwrite the metric? Is this because ONLY_OUTPUT is added generically (when there's one output) and then the subclass uses self.tagged_receivers to do it "properly" later? > I discovered that the metric was created twice, because I am not using a map > of tags I am just adding another entry when the metric is added as a > monitoring_info a second time. We should probably clarify what the contract is here. I think I was assuming a "create or return" semantics when you get a metric for a fully qualified name. > So if this is intentional, then I need to make my code do the equivalent > thing, and check that there is already a MonitoringInfo for the metric and > update its value (or assert it is the same value). > > Also, is it intentional to use None as a tag name here? Seems like an odd > choice. None (kind of like Java's NULL) is used when an output name is not provided. The protos only accept strings here though, hence 'None'.
