Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Instrument Solver Engine #2129

Merged
merged 8 commits into from
Dec 8, 2023
Merged

Instrument Solver Engine #2129

merged 8 commits into from
Dec 8, 2023

Conversation

fleupold
Copy link
Contributor

@fleupold fleupold commented Dec 6, 2023

Description

Solver engines are currently not exposing any metrics. This PR changes that.

Changes

  • Collect histogram of initial time_limit when requests come in
  • Collect histogram of remaining time when requests are done
  • Collect number of proposed solutions
  • Collect failure reasons

How to test

Run everything locally and visit http://localhost:7872/metrics

Once merged, this will require an infra change to start scraping metrics in prometheus.

Related Issues

Fixes #1239

@fleupold fleupold requested a review from a team as a code owner December 6, 2023 16:56
crates/solvers/src/infra/observe/mod.rs Outdated Show resolved Hide resolved
crates/solvers/src/api/routes/metrics.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@MartinquaXD MartinquaXD left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only nits.

crates/solvers/src/infra/metrics.rs Outdated Show resolved Hide resolved
Comment on lines +27 to +28
prometheus = { workspace = true }
prometheus-metric-storage = { workspace = true }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My plan didn't pan out entirely. 🙁
Maybe we can reexport the types we need from those crates in the observe crate as well in another PR. Then observe should be the only crate that directly depends on them.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not able to get this to compile. The proc macro that generates the Metrics struct requires prometheus_metric_storage so even with re-exports I'm getting

error[E0433]: failed to resolve: use of undeclared crate or module prometheus_metric_storage
--> crates/solvers/src/infra/metrics.rs:7:24
|
7 | #[derive(Debug, Clone, metrics::MetricStorage)]
| ^^^^^^^^^^^^^^^^^^^^^^ use of undeclared crate or module prometheus_metric_storage
|
= note: this error originates in the derive macro metrics::MetricStorage (in Nightly builds, run with -Z macro-backtrace for more info)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feel free to merge the PR without this. It would just be a nice clean up that can happen at any time and should not delay proper metrics on the solvers crate.
That being said I remember exporting macros requiring a specific macro on top of it and reexporting a macro might be even more magical. 😄 🤷‍♂️

crates/solvers/src/infra/metrics.rs Outdated Show resolved Hide resolved
@fleupold fleupold enabled auto-merge (squash) December 8, 2023 17:23
@fleupold fleupold merged commit 03ed19b into main Dec 8, 2023
8 checks passed
@fleupold fleupold deleted the instrument_solver_engine branch December 8, 2023 19:32
@github-actions github-actions bot locked and limited conversation to collaborators Dec 8, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

solvers Crate Instrumentation
3 participants