Let's put these behind a feature-flag that

• We keep off by default (for now)
• We enable at Anyscale to test/verify
• (If pass previous steps) Enable globally in the next release

Let's make sure this is consistent with tags on Core dashboard

Where is the best place to check for consistency of tags? Perhaps somewhere in here https://github.com/ray-project/ray/blob/master/python/ray/dashboard/modules/metrics/dashboards/default_dashboard_panels.py?

Yeah, just look at the Core dashboard

Ok did a bit more digging. The core dashboard uses instance which is a label that is automatically added when generating prometheus metrics (described here). That is what I initially tried to use for our metrics reporting, but the problem was that all of our metrics are reported from the same head node (which is why I went down the path of adding the node id / node ip to our metrics). I think it would be more confusing to overwrite this instance label but that would lead to drift in naming.

ChatGPT summary of implications of overwriting internal labels (like instance):

In Prometheus, the instance label is not strictly “reserved” in the sense that you cannot overwrite it; technically, you can overwrite or redefine any label. However, the instance label has a conventional meaning (identifying the target host/port), and Prometheus automatically adds it based on the scrape target configuration. Overwriting it in your metrics (or via relabeling) has some caveats:

Loss of Original Target Identity
If you overwrite the instance label yourself, Prometheus and Grafana might no longer be able to distinguish metrics based on the actual scrape target. This can create confusion if you rely on queries or dashboards that expect instance to be the unique identifier for each host or service.

Clashes with the Scrape-Assigned instance
When Prometheus scrapes a target, it adds instance=":" automatically. If your exported metric also includes instance="<some_value>", you can end up with label collisions. Prometheus merges “external” labels and “internal” labels, so you might see unexpected results unless you’ve explicitly configured a relabeling rule that changes or drops one of the labels.

Best Practice: Avoid Overwriting
Since instance is a canonical label for many setups (used by standard exporters and many default dashboards), it’s typically best practice to leave instance intact. If you want to attach some other identifying label (e.g., node_id, env, region, etc.), add a new label instead of overloading instance.

If You Must Overwrite

Use relabeling in prometheus.yml (under scrape_configs) to transform the instance label to something else or to attach additional metadata.
Be aware that queries referencing instance might break or need to be updated.
Document the reason you’re overriding it to avoid surprises for others looking at the metrics.

Makes sense. Let's do that then

1. Make sure we also identify by instance ip
2. We can later consider amending vector to actually use rewrite node_ip to instance

Added the blocks output but kept the "of_finished_tasks". We have a few different bytes output metrics and this directly corresponds to another existing metric.

@omatthew98 what are the other metric you're referring to?

Somehow I edited your comment rather than responding lol, fixing but what I said was:
At the very least we have bytes_task_outputs_generated and bytes_outputs_of_finished_tasks. In stats.py we refer to bytes_task_outputs_generated as just output_bytes so don't want to conflate those two (gauge defined here and update here).

These will be node-ids that produced the blocks, not the ones where it will be executing

Not sure if I understand the significance of this, does this mean all the input data is wrong because these will not be updates as subsequent blocks are produced / processed by downstream operators? Is there something different I should be using here?

You're making an assumption that the task will run where the block is located

• This holds for output
• This does NOT necessarily hold for inputs

This method seems to be doing the opposite of its name -- aggregating per-node metrics

Can inline this, this isn't gonna be changing often

No. num_outputs == 0 means this is the first time we receive an output from this task, thus bump num_tasks_have_outputs.

ditto

nit, add a util function for this repeated code pattern:

def iter_meta_and_node_metrics(ref_bundle):
    for _, meta in output.blocks:
        # ...
        yield meta, node_metrics

nit, add a util function for this repeated code pattern:

def iter_meta_and_node_metrics(ref_bundle):
    for _, meta in output.blocks:
        # ...
        yield meta, node_metrics

what does this change mean?

The original is a single two-item tuple. The second indicates a variable-length tuple of such two-item tuples. If I have misunderstood the type I can change it back.

I see. didn't know about this syntax. maybe let's add a comment here.

use the constant. maybe just move the entire function to op_runtime_metrics.py

this should also be guarded by the flag?

Yes, good catch.

should we enable this by default? how expensive is it?

I was going off of Alexey's plan here #49705 (comment). I haven't measured directly how expensive it is yet.

@raulchen this will considerably increase cardinality of some metrics, i'd much rather take a slower road here of flipping it on the platform first, observing it then flipping on in OSS

@omatthew98 let's make this configurable by env-var

because it will create a new tag for each node?
Sounds that it will easily hit the prometheus limit for large clusters
Maybe we should defer to the deployment to decide whether to enable this flag based on cluster side. Let's add a comment here.

nit, simplify this with a loop:

for attr in dir(
    agg_node_metrics[attr] += getattr(node_metrics, attr)

@omatthew98 what are the other metric you're referring to?

@raulchen this will considerably increase cardinality of some metrics, i'd much rather take a slower road here of flipping it on the platform first, observing it then flipping on in OSS

@omatthew98 let's make this configurable by env-var

Yeah, just look at the Core dashboard

That's actually a great idea to assure consistency of the metrics even in cases when metadata won't be available!

Let's extract to a common util

You're not adding to node_ids_seen. Also, why do we need it?

Think I accidentally removed in a refactor, thanks for catching. This is used to keep track of the nodes that we have already incremented per node so that we don't double count (as they should only be incremented once per node per RefBundle although there might be more than one block with the same node id).

Let's abstract this to a method inside Metrics class itself (include L180 in it as well)

This will be double-counting blocks (in in-queue and out-queue)

@alexeykudinkin could you help me understand how this double counts?

Can we seal this data context (i.e., create a self._data_context attribute from the context passed into the constructor, and then use that)?

Moved this code to op_runtime_metrics.py but also switched to pulling the context from the operator directly as was suggested by @alexeykudinkin.

Any reason we need to use field here?

Good call, no reason, will remove / simplify them all.

We count bundles in an operator's internal inqueue towards the previous operator's budget. So, if you're trying to reason about budgets and backpressure, counting these toward the current operator might be confusing.

I think it might be more useful to mirror that logic here and exclude internal inqueues from an operator's estimate, but @raulchen curious to get your thoughts.

As an aside, I think it'd be good to have more explicit abstractions around bundle ownership (out of scope for this PR)

@alexeykudinkin could you help me understand how this double counts?

Is this double-counting internal inqueues? Like, we add the memory usage from internal inqueue blocks in two places: for the current operator, and the previous oeprator's?

I'm confused about what num_tasks_running represents here.

To provide a concrete example, let's say inputs is a bundle with two blocks. One block was created on node "A", and another node was created on node "B". And then the submitted task gets scheduled on node "C".

In this case, would it show that nodes "A" and "B" each have a task running, and no tasks are running on node "C"? If so, I think that'd be really unintuitive

In a similar vein as my comment about num_tasks_running, I'm not really sure how I'd interpret num_tasks_finished. If I'm understanding correctly, it represents the number of tasks finished that have an input from this node, rather than the number of finished tasks that ran on this node.