-
Notifications
You must be signed in to change notification settings - Fork 28
Effect API metrics + latency metrics + Dev Console with disabled TUI #795
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -299,7 +299,9 @@ type effect = { | |||||||||||||||||
| cache: option<effectCacheMeta>, | ||||||||||||||||||
| output: S.t<effectOutput>, | ||||||||||||||||||
| input: S.t<effectInput>, | ||||||||||||||||||
| mutable callsCount: int, | ||||||||||||||||||
| // The number of functions that are currently running. | ||||||||||||||||||
| mutable activeCallsCount: int, | ||||||||||||||||||
| mutable prevCallStartTimerRef: Hrtime.timeRef, | ||||||||||||||||||
| } | ||||||||||||||||||
|
Comment on lines
+302
to
305
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chainprevCallStartTimerRef should be optional; current design invites null-sentinel misuse Field is typed as Hrtime.timeRef but Envio.res initializes it with raw null. Without an option type, downstream code can crash if it dereferences without a guard. Recommended change: - mutable prevCallStartTimerRef: Hrtime.timeRef,
+ mutable prevCallStartTimerRef: option<Hrtime.timeRef>,Then initialize with None in Envio.res and handle Some(_) where used. This removes %raw(null) and restores type safety. If keeping non-optional, provide a non-null sentinel from Hrtime (e.g., Hrtime.makeTimer()) and ensure all sites don’t assume null. Check usages and legacy name: 🏁 Script executed: #!/bin/bash
# Ensure all uses guard null/option appropriately
rg -nP -C3 'prevCallStartTimerRef'
# Ensure no stale references to callsCount remain
rg -nP -C2 '\bcallsCount\b'Length of output: 3874 Change The field is initialized with Apply the recommended change: - mutable prevCallStartTimerRef: Hrtime.timeRef,
+ mutable prevCallStartTimerRef: option<Hrtime.timeRef>,Update initialization in Envio.res:72 to 📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||
| let cacheTablePrefix = "envio_effect_" | ||||||||||||||||||
| let cacheOutputSchema = S.json(~validate=false)->(Utils.magic: S.t<Js.Json.t> => S.t<effectOutput>) | ||||||||||||||||||
|
|
||||||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -70,14 +70,7 @@ module Benchmark = { | |||||||||||||||||||||||
| | _ => false | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| let shouldSavePrometheus = self => | ||||||||||||||||||||||||
| switch self { | ||||||||||||||||||||||||
| | Prometheus => true | ||||||||||||||||||||||||
| // Always save benchmarks in Prometheus in TUI mode | ||||||||||||||||||||||||
| // This is needed for Dev Console profiler | ||||||||||||||||||||||||
| // FIXME: This doesn't take into account the arg from the CLI | ||||||||||||||||||||||||
| | _ => !tuiOffEnvVar | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
| let shouldSavePrometheus = _ => true | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| let shouldSaveData = self => self->shouldSavePrometheus || self->shouldSaveJsonFile | ||||||||||||||||||||||||
|
Comment on lines
+73
to
75
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chainForcing Prometheus “on” ignores ENVIO_SAVE_BENCHMARK_DATA and breaks config expectations This unconditionally enables Prometheus saving, making SaveDataStrategy.shouldSaveData always true regardless of user input. Likely unintended and a breaking behavior change for users who disable metrics or only want json-file. Suggested fix (restore gating by variant and Bool(true)): - let shouldSavePrometheus = _ => true
+ let shouldSavePrometheus = self =>
+ switch self {
+ | Prometheus
+ | Bool(true) => true
+ | _ => false
+ }Optionally, if “always-on Prometheus” is truly desired, please:
Run to confirm no other code assumes shouldSaveData can be false: 🏁 Script executed: #!/bin/bash
rg -nP --type=res -C2 '\bBenchmark\.shouldSaveData\b|SaveDataStrategy\.shouldSaveData'Length of output: 27 🏁 Script executed: #!/bin/bash
# Search for shouldSaveData usage in ReScript files
rg -nP -C2 'shouldSaveData' --glob='*.res'Length of output: 3349 🏁 Script executed: #!/bin/bash
# Find SaveDataStrategy type definition and its variants
rg -nP -A10 'type.*SaveDataStrategy\b|type t\s*=|^[[:space:]]*(Bool|Prometheus|Json)' codegenerator/cli/templates/static/codegen/src/Env.res | head -50Length of output: 343 🏁 Script executed: #!/bin/bash
# Find ENVIO_SAVE_BENCHMARK_DATA references to confirm environment variable mapping
rg -nP 'ENVIO_SAVE_BENCHMARK_DATA|saveDataStrategy' codegenerator/cli/templates/static/codegen/src/Env.res | head -30Length of output: 202 Forcing Prometheus "on" unconditionally breaks SaveDataStrategy configuration The change at line 73 forces Restore gating by SaveDataStrategy variant: - let shouldSavePrometheus = _ => true
+ let shouldSavePrometheus = self =>
+ switch self {
+ | Prometheus
+ | Bool(true) => true
+ | _ => false
+ }📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
Avoid %raw(null); align with an option-typed ref or a valid default
prevCallStartTimerRef is set via %raw(
null) while typed as Hrtime.timeRef. Unsafe and bypasses the type system; may crash if used unguarded.Based on learnings.
🏁 Script executed:
Length of output: 27
Length of output: 230
🏁 Script executed:
Length of output: 3874
🏁 Script executed:
Length of output: 230
Length of output: 44
🏁 Script executed:
Length of output: 146
🏁 Script executed:
Length of output: 1495
🏁 Script executed:
Length of output: 464
Avoid %raw(
null); align with an option-typed ref or a valid defaultThe concern is valid.
prevCallStartTimerRefis initialized with%raw(\null`)at line 72 in Envio.res while typed as the non-optionalHrtime.timeRef` in Internal.res, which bypasses type safety.Although current usage in LoadLayer.res guards against reading null on the first call (
if hadActiveCallsat line 141), this fragile defensive programming should be replaced with proper typing:option<Hrtime.timeRef>and initialize withNone, orHrtime.timeRefdefault (e.g.,Hrtime.makeTimer())🤖 Prompt for AI Agents