[core] Add metrics for gcs jobs

[dentiny]

Why are these changes needed?

This PR adds metrics for job states within job manager.

In detail, a gauge stats is sent via opencensus exporter, so running ray jobs could be tracked and alerts could be created later on.

Fault tolerance is not considered, according to doc, state is re-constructed at restart.

On testing, the best way is to observe via opencensus backend (i.e. google monitoring dashboard), but not easy for open-source contributors; or to have a mock / fake exporter implementation, which I don't find in the code base.

Related issue number

Closes #47438

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

[dentiny]

Creating a shared pointer with new leads to two allocation, while only once for std::make_shared.
Ref: modern effective C++ item 21

[dentiny]

google coding style has recommendation for declaration order: https://google.github.io/styleguide/cppguide.html#Declaration_Order

[rynewang]

This PR does not consider HA situation. "state is re-constructed at restart" yes but this needs GcsJobManager's own code. In GcsJobManager::Initialize you receive a redis snapshot of the prev state. Since you added a new state running_job_ids_ you need to init it in that method.

We also have job_id in the metrics. This means each job creates a metric stream which gives up cardinality problems. I am thinking if we only need a single {State} without job ID.

The metrics is updated every when a job starts/ends. I guess we can instead expose that periodically, in align with the other managers' RecordMetrics method as in

ray/src/ray/gcs/gcs_server/gcs_server.cc

Line 792 in dde49e4

gcs_task_manager_->RecordMetrics();

.

[jjyao]

No need to have JobId label: we just want two time series: one for running and one for finished.

[dentiny]

Updated.

Edit: I split the job status into two parts:

• Gauge for running jobs;
• Counter for finished jobs.

[jjyao]

Oh, sorry for the confusing. What I meant is that we still have one stats called jobs but have a State label with cardinality 2 (RUNNING and FINISHED). This will be stored as two time series in prometheus.

[rynewang]

Note: maybe you don't need to save all running_job_ids since you can keep both as a counter; and when the counter resets on gcs restart Prometheus can take care of the reset.

[dentiny]

maybe you don't need to save all running_job_ids since you can keep both as a counter

Well you mentioned HandleAddJob needs to be idempotent (which I guess it could be invoked repeatedly due to reasons like retry), so if we use an integer value here, incrementing the value directly leads to mis-report.

The reason why I use set for running task count (gauge) and integer for finished task (counter) is:

• Gauge is queried on current value, it better to be precise (i.e. having 1 running job vs 0 running job differs a lot)
• Counter is queried via rate (as you mentioned above), which we care more about the rate of increment/decrement, rather than an absolute value

[dentiny]

This PR does not consider HA situation. "state is re-constructed at restart" yes but this needs GcsJobManager's own code. In GcsJobManager::Initialize you receive a redis snapshot of the prev state. Since you added a new state running_job_ids_ you need to init it in that method.

Thanks for the code pointer! State recovery logic added.

We also have job_id in the metrics. This means each job creates a metric stream which gives up cardinality problems. I am thinking if we only need a single {State} without job ID.

Sounds good, I'm curious about the cardinality for jobs?

Terminology wise, job is a collection of tasks.
My personal experience, as for AV company, it's not uncommon to have tens of thousands of tasks running concurrently at the same time, but job number is usually 100-200 per day.

But yeah removing the job id sounds good.

The metrics is updated every when a job starts/ends. I guess we can instead expose that periodically, in align with the other managers' RecordMetrics method as in

Thanks for the code pointer! Updated to unify metrics report.

[alanwguo]

@dentiny @jjyao is there a ticket for the follow-up of having a metric to track job state changes. We have a user which would like to alert whenever a ray job fails.