fix: add metrics for wf engine #2076

MasterPtato · 2025-02-25T02:47:46Z

Changes

MasterPtato · 2025-02-25T02:48:28Z

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

How to use the Graphite Merge Queue

Add the label merge-queue to this PR to add it to the merge queue.

You must have a Graphite account in order to use the merge queue. Sign up using this link.

_{An organization admin has enabled the Graphite Merge Queue in this repository.} _{Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.}

This stack of pull requests is managed by Graphite. Learn more about stacking.

cloudflare-workers-and-pages · 2025-02-25T02:52:54Z

Deploying rivet with Cloudflare Pages

Latest commit:	`dbc2e30`
Status:	🚫 Build failed.

View logs

greptile-apps

PR Summary

This PR adds comprehensive metrics tracking to the workflow engine. Here's a summary of the key changes:

Added timing metrics for workflow operations including signal dispatch, message handling, and sub-workflow execution using Instant::now() measurements
Added new metrics in metrics.rs for tracking workflow states, signal handling latencies, and message processing durations with proper labeling
Added SQLite performance testing module with worker tasks to measure database operation latencies
Changed SQLite synchronization mode from Normal to Full for better data consistency, trading some performance for reliability
Added proper error handling and metric recording across workflow operations with standardized metric naming conventions

The changes appear well-structured and improve observability of the workflow engine's performance. However, there are a few potential concerns:

The SQLite synchronization mode change could impact performance and should be carefully monitored
Some metric labels use empty strings which may affect metric cardinality and querying
The large number of changed files suggests this PR may be mixing multiple concerns beyond just metrics
Some debug logging statements look like temporary development code and should be cleaned up

_{64 file(s) reviewed, 19 comment(s)}
_{Edit PR Review Bot Settings | Greptile}

greptile-apps · 2025-02-25T02:48:38Z

packages/common/chirp-workflow/core/Cargo.toml

 [dev-dependencies]
 anyhow = "1.0.82"
-rand = "0.8"
+rand = "0.8"
+statrs = "0.18"
+dirs = "5.0.1"


style: Duplicate dependency anyhow defined in both dependencies and dev-dependencies sections. Consider removing from dev-dependencies since it's already available through main dependencies.

Suggested change

[dev-dependencies]

anyhow = "1.0.82"

rand = "0.8"

\ No newline at end of file

rand = "0.8"

statrs = "0.18"

dirs = "5.0.1"

[dev-dependencies]

rand = "0.8"

statrs = "0.18"

dirs = "5.0.1"

greptile-apps · 2025-02-25T02:48:40Z

packages/common/chirp-workflow/core/src/builder/common/signal.rs

+			.with_label_values(&["", T::NAME])
+			.observe(dt);


style: Empty string label seems unnecessary and inconsistent. Consider removing the empty string or documenting why it's needed.

greptile-apps · 2025-02-25T02:48:40Z

packages/common/chirp-workflow/core/src/builder/common/signal.rs

+		let dt = start_instant.elapsed().as_secs_f64();
+		metrics::SIGNAL_SEND_DURATION
+			.with_label_values(&["", T::NAME])
+			.observe(dt);
 		metrics::SIGNAL_PUBLISHED


logic: Duration metric should be recorded before incrementing the published counter in case of failures

greptile-apps · 2025-02-25T02:48:57Z

packages/common/chirp-workflow/core/src/builder/common/message.rs

+			.with_label_values(&["", M::NAME])
+			.observe(dt);


style: Empty string label prefix seems incorrect. Should document why this empty string is needed or remove it if unnecessary.

greptile-apps · 2025-02-25T02:48:58Z

packages/common/chirp-workflow/core/src/builder/common/message.rs

+		let dt = start_instant.elapsed().as_secs_f64();
+		metrics::MESSAGE_SEND_DURATION
+			.with_label_values(&["", M::NAME])
+			.observe(dt);
 		metrics::MESSAGE_PUBLISHED
-			.with_label_values(&[M::NAME])
+			.with_label_values(&["", M::NAME])
 			.inc();



logic: Metrics are recorded even if message send fails. Consider moving metrics recording into the success path only.

greptile-apps · 2025-02-25T02:51:50Z

packages/common/pools/src/db/sqlite/keys.rs

 pub struct DbDataKey {
-	db_name_segment: Arc<Vec<u8>>
+	db_name_segment: Arc<Vec<u8>>,
 }


style: The struct field visibility is private but the struct itself is public. Consider documenting why this design choice was made or making the field public if it needs to be accessed externally.

greptile-apps · 2025-02-25T02:52:50Z

packages/common/pools/src/db/sqlite/tests.rs

-    root.server.as_mut().unwrap().foundationdb = Some(Default::default());
-    let config = rivet_config::Config::from_root(root);
+	let mut root = rivet_config::config::Root::default();
+	root.server.as_mut().unwrap().foundationdb = Some(Default::default());


logic: unwrap() on Option without error handling could panic if server is None

Suggested change

root.server.as_mut().unwrap().foundationdb = Some(Default::default());

root.server.get_or_insert_with(Default::default).foundationdb = Some(Default::default());

greptile-apps · 2025-02-25T02:53:39Z

packages/common/server-cli/src/util/wf/mod.rs

+			let date = if print_ts > 1 {
+				datetime.format("%Y-%m-%d %H:%M:%S%.3f")
+			} else {
+				datetime.format("%Y-%m-%d %H:%M:%S")
+			};


style: Consider extracting timestamp formatting logic into a helper function since it's repeated in multiple places

greptile-apps · 2025-02-25T02:57:08Z

packages/edge/services/pegboard/src/metrics.rs

+
+	pub static ref INSERT_COMMANDS_ACQUIRE_DURATION: HistogramVec = register_histogram_vec_with_registry!(
+		"pegboard_client_insert_commands_acquire_duration",
+		"TODO REMOVE",


style: The description 'TODO REMOVE' suggests this metric is temporary but doesn't explain why. Either remove the metric now or document its actual purpose.

greptile-apps · 2025-02-25T02:57:09Z

packages/edge/services/pegboard/src/metrics.rs

+		"pegboard_client_insert_commands_acquire_duration",
+		"TODO REMOVE",
+		&["workflow_id"],
+		BUCKETS.to_vec(),


style: BUCKETS.to_vec() creates a new vector allocation for each histogram. Consider using BUCKETS directly if possible to avoid unnecessary allocations.

cloudflare-workers-and-pages · 2025-02-26T06:18:19Z

Deploying rivet-hub with Cloudflare Pages

Latest commit:	`dba8d32`
Status:	✅ Deploy successful!
Preview URL:	https://63951b92.rivet-hub-7jb.pages.dev
Branch Preview URL:	https://02-25-fix-add-metrics-for-wf.rivet-hub-7jb.pages.dev

View logs

MasterPtato requested a review from NathanFlurry February 25, 2025 02:47

greptile-apps bot reviewed Feb 25, 2025

View reviewed changes

MasterPtato force-pushed the 02-25-fix_add_metrics_for_wf_engine branch from ef7f175 to 51a9774 Compare February 25, 2025 22:10

NathanFlurry force-pushed the 02-25-fix_add_metrics_for_wf_engine branch from 51a9774 to dba8d32 Compare February 26, 2025 01:05

NathanFlurry force-pushed the 02-22-fix_various_bug_fixes branch from 4eff98d to ed680a5 Compare February 26, 2025 01:05

NathanFlurry mentioned this pull request Feb 26, 2025

chore(pools): rewrite sqlite manager to run without locks using scc #2082

Open

fix: add metrics for wf engine

dbc2e30

MasterPtato force-pushed the 02-22-fix_various_bug_fixes branch from ed680a5 to 2051a35 Compare February 26, 2025 03:29

MasterPtato force-pushed the 02-25-fix_add_metrics_for_wf_engine branch from dba8d32 to dbc2e30 Compare February 26, 2025 03:29

MasterPtato mentioned this pull request Feb 26, 2025

feat: add grafana to docker compose #2083

Open

NathanFlurry force-pushed the 02-22-fix_various_bug_fixes branch from 2051a35 to ed680a5 Compare February 26, 2025 06:18

NathanFlurry force-pushed the 02-25-fix_add_metrics_for_wf_engine branch from dbc2e30 to dba8d32 Compare February 26, 2025 06:18

MasterPtato force-pushed the 02-22-fix_various_bug_fixes branch from ed680a5 to 2051a35 Compare February 27, 2025 02:45

MasterPtato force-pushed the 02-25-fix_add_metrics_for_wf_engine branch from dba8d32 to dbc2e30 Compare February 27, 2025 02:45

MasterPtato mentioned this pull request Feb 27, 2025

chore: optimizations and such #2086

Open

NathanFlurry force-pushed the 02-25-fix_add_metrics_for_wf_engine branch from dbc2e30 to dba8d32 Compare February 27, 2025 07:59

NathanFlurry force-pushed the 02-22-fix_various_bug_fixes branch from 2051a35 to ed680a5 Compare February 27, 2025 07:59

This was referenced Feb 27, 2025

chore(hub-embed): disable all node-related code if hub building is disabled #2084

Open

chore(runtime): add otel support to runtime #2085

Open

fix(runtime): remove forcing tokio core count to 2 #2087

Open

MasterPtato force-pushed the 02-22-fix_various_bug_fixes branch from ed680a5 to 2051a35 Compare February 27, 2025 18:58

MasterPtato force-pushed the 02-25-fix_add_metrics_for_wf_engine branch from dba8d32 to dbc2e30 Compare February 27, 2025 18:58

NathanFlurry force-pushed the 02-22-fix_various_bug_fixes branch from 2051a35 to ed680a5 Compare February 27, 2025 20:45

NathanFlurry force-pushed the 02-25-fix_add_metrics_for_wf_engine branch from dbc2e30 to dba8d32 Compare February 27, 2025 20:46

NathanFlurry mentioned this pull request Feb 27, 2025

fix(docker): auto-create otel database #2088

Open

MasterPtato force-pushed the 02-22-fix_various_bug_fixes branch from ed680a5 to 2051a35 Compare February 28, 2025 03:08

MasterPtato force-pushed the 02-25-fix_add_metrics_for_wf_engine branch from dba8d32 to dbc2e30 Compare February 28, 2025 03:08

MasterPtato mentioned this pull request Feb 28, 2025

chore: combine ds into pb #2089

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: add metrics for wf engine #2076

fix: add metrics for wf engine #2076

MasterPtato commented Feb 25, 2025

MasterPtato commented Feb 25, 2025 •

edited

Loading

cloudflare-workers-and-pages bot commented Feb 25, 2025 •

edited

Loading

greptile-apps bot left a comment

greptile-apps bot Feb 25, 2025

greptile-apps bot Feb 25, 2025

greptile-apps bot Feb 25, 2025

greptile-apps bot Feb 25, 2025

greptile-apps bot Feb 25, 2025

greptile-apps bot Feb 25, 2025

greptile-apps bot Feb 25, 2025

greptile-apps bot Feb 25, 2025

greptile-apps bot Feb 25, 2025

greptile-apps bot Feb 25, 2025

cloudflare-workers-and-pages bot commented Feb 26, 2025 •

edited

Loading

	root.server.as_mut().unwrap().foundationdb = Some(Default::default());
	root.server.get_or_insert_with(Default::default).foundationdb = Some(Default::default());

fix: add metrics for wf engine #2076

Are you sure you want to change the base?

fix: add metrics for wf engine #2076

Conversation

MasterPtato commented Feb 25, 2025

Changes

MasterPtato commented Feb 25, 2025 • edited Loading

How to use the Graphite Merge Queue

cloudflare-workers-and-pages bot commented Feb 25, 2025 • edited Loading

Deploying rivet with Cloudflare Pages

greptile-apps bot left a comment

Choose a reason for hiding this comment

PR Summary

greptile-apps bot Feb 25, 2025

Choose a reason for hiding this comment

greptile-apps bot Feb 25, 2025

Choose a reason for hiding this comment

greptile-apps bot Feb 25, 2025

Choose a reason for hiding this comment

greptile-apps bot Feb 25, 2025

Choose a reason for hiding this comment

greptile-apps bot Feb 25, 2025

Choose a reason for hiding this comment

greptile-apps bot Feb 25, 2025

Choose a reason for hiding this comment

greptile-apps bot Feb 25, 2025

Choose a reason for hiding this comment

greptile-apps bot Feb 25, 2025

Choose a reason for hiding this comment

greptile-apps bot Feb 25, 2025

Choose a reason for hiding this comment

greptile-apps bot Feb 25, 2025

Choose a reason for hiding this comment

cloudflare-workers-and-pages bot commented Feb 26, 2025 • edited Loading

Deploying rivet-hub with Cloudflare Pages

MasterPtato commented Feb 25, 2025 •

edited

Loading

cloudflare-workers-and-pages bot commented Feb 25, 2025 •

edited

Loading

cloudflare-workers-and-pages bot commented Feb 26, 2025 •

edited

Loading