[Data] Refactor OpRuntimeMetrics to support properties

[bveeramani]

Why are these changes needed?

Metrics that are implemented as properties (e.g., average_bytes_inputs_per_task) aren't shown on the Ray Data dashboard. This PR refactors the implementation of OpRuntimeMetrics to fix the issue.

Additional details: _StatsActor uses fields (a function that returns all of the fields in a dataclass) to generate a list of metrics to show in the Dashboard.

ray/python/ray/data/_internal/stats.py

Lines 266 to 280 in 1c80db5

	def _create_prometheus_metrics_for_execution_metrics(
	self, metrics_group: str, tag_keys: Tuple[str, ...]
	) -> Dict[str, Gauge]:
	metrics = {}
	for field in fields(OpRuntimeMetrics):
	if not field.metadata.get("metrics_group") == metrics_group:
	continue
	metric_name = f"data_{field.name}"
	metric_description = field.metadata.get("description")
	metrics[field.name] = Gauge(
	metric_name,
	description=metric_description,
	tag_keys=tag_keys,
	)
	return metrics

This is an issue because _StatsActor is assumes that 1) all metrics are implemented as fields, and 2) all fields represent metrics. This refactor makes the interface more explicit by introducing a OpRuntimeMetrics.get_metrics method.

Related issue number

Checks

• I've signed off every commit(by using the -s flag, i.e., git commit -s) in this PR.
• I've run scripts/format.sh to lint the changes in this PR.
• I've included any doc changes needed for https://docs.ray.io/en/master/.
  • I've added any new APIs to the API Reference. For example, if I added a
    method in Tune, I've added it in doc/source/tune/api/ under the
    corresponding .rst file.
• I've made sure the tests are passing. Note that there might be a few flaky tests, see the recent failures at https://flakey-tests.ray.io/
• Testing Strategy
  • Unit tests
  • Release tests
  • This PR is not tested :(

[bveeramani]

Dead code.

[bveeramani]

Dead code.

[raulchen]

is it possible to just add the metric to the list in metricfield. So we don't need this meta class?

[scottjlee]

alternatively, i think we can add that logic in OpRuntimeMetrics.__post_init__

[bveeramani]

How would we get the name of the attribute in metricfield? Field.name is None in metricfield. I'm not sure how name exactly gets set, but I think it's some dataclass magic that occurs after the class is defined.

One alternative is to explicitly specify the name. Advantage is that we don't have to use metaclasses; disadvantage is that you need to duplicate the name:

num_inputs_received: int = metricfield(
        name="num_inputs_received",
        default=0,
        description="Number of input blocks received by operator.",
        metrics_group="inputs",
    )

[scottjlee]

personally, i prefer to only specify the name only when needed. if you put the logic in __post_init__, that should have a valid name attribute i think?

[bveeramani]

alternatively, i think we can add that logic in OpRuntimeMetrics.post_init

We could do this. We'd just need to make sure we don't add duplicate items since post_init is called once per instance and not once when the class is defined.

[bveeramani]

Don't have a strong preference for post_init vs. metaclass. Down for either

[scottjlee]

Discussed offline, we propose putting the logic in post_init, and making _METRICS a set so that we don't keep duplicates.

[bveeramani]

After testing it out, I think __post_init__ might not work. __post_init__ only works if you don't override __init__, and we currently override __init__ to pass the Operator to the OpRuntimeMetrics instance.

[alexeykudinkin]

We're adding to _METRICS twice

[bveeramani]

@alexeykudinkin wdym?

[alexeykudinkin]

These are metric definitions not metrics themselves. Let's make that clear from the description

[bveeramani]

@alexeykudinkin OpRuntimeMetrics contains the metric definitions as well as the metric values. What's the difference between what's in OpRuntimeMetrics and what you mean by metric?