chore: migrate to new deno_core and metrics #21057
Conversation
mmastrac
commented
Nov 2, 2023
•
mmastrac
commented
- Uses the new OpMetrics system for sync and async calls
- Partial revert of feat: enable Array.fromAsync #21048 as we moved Array.fromAsync upstream to deno_core
|
/bench http[minimal,realistic] |
|
Benchmark failed! 😵 start: id: server: |
|
/bench http[minimal,realistic] |
http[minimal]
http[realistic]
start: id: server: |
| // Copyright 2018-2023 the Deno authors. All rights reserved. MIT license. | ||
| import { assert, assertEquals } from "./test_util.ts"; | ||
|
|
||
| Deno.test(async function metrics() { |
There was a problem hiding this comment.
This is tested in deno_core now.
| @@ -412,19 +412,6 @@ Deno.test(function clearTimeoutAndClearIntervalNotBeEquals() { | |||
| assertNotEquals(clearTimeout, clearInterval); | |||
| }); | |||
|
|
|||
| Deno.test(async function timerMaxCpuBug() { | |||
There was a problem hiding this comment.
I couldn't think of a good way to replicate this test without using metrics... thoughts?
mmastrac
force-pushed
the
deno_core_update
branch
from
e727161
to
2b8e654
Compare
| @@ -3425,11 +3425,6 @@ itest!(unstable_ffi_19 { | |||
| exit_code: 70, | |||
| }); | |||
|
|
|||
| itest!(future_check2 { | |||
There was a problem hiding this comment.
Was effectively doing and testing nothing.
| @@ -398,6 +398,7 @@ pub struct Flags { | |||
| pub config_flag: ConfigFlag, | |||
| pub node_modules_dir: Option<bool>, | |||
| pub vendor: Option<bool>, | |||
| pub enable_op_summary_metrics: bool, | |||
There was a problem hiding this comment.
This is a flag but isn't currently exposed on the CLI args yet.
runtime/worker.rs
Outdated
| let op_summary_metrics = Rc::new(OpMetricsSummaryTracker::default()); | ||
| deno_core::extension!(deno_op_metrics, | ||
| options = { | ||
| op_summary_metrics: Rc<OpMetricsSummaryTracker> | ||
| }, | ||
| state = |state, options| { | ||
| state.put(options.op_summary_metrics) | ||
| } | ||
| ); |
There was a problem hiding this comment.
Is it only for MainWorker? What about WebWorker?
Also seems strange to define an extension to put a default object in the state. Maybe do it directly after creating JsRuntime?
There was a problem hiding this comment.
I added code for WebWorker as well -- the metrics there are never fetched (AFAICT) but I suppose it's good for consistency.
I switched it up to manually add it to the JsRuntime after as suggested but deno_permissions_worker is also doing something similar. I'm OK either way.
