Add prometheus metric for effect calls #582
Conversation
WalkthroughThe changes introduce tracking for the number of times an effect is called, both locally within the effect structure and via Prometheus metrics. New fields and modules are added to support this, including a mutable Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant LoadLayer
participant Effect
participant Prometheus
Caller->>LoadLayer: Call loadEffect(args)
LoadLayer->>Effect: Increment callsCount by args.length
LoadLayer->>Prometheus: EffectCallsCount.set(callsCount, effectName)
LoadLayer->>Effect: Invoke effect handler
LoadLayer-->>Caller: Return result
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
💤 Files with no reviewable changes (1)
✅ Files skipped from review due to trivial changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
codegenerator/cli/npm/envio/src/Envio.res(1 hunks)codegenerator/cli/npm/envio/src/Internal.res(1 hunks)codegenerator/cli/npm/envio/src/Prometheus.res(3 hunks)codegenerator/cli/templates/static/codegen/src/LoadLayer.res(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build_and_test
🔇 Additional comments (7)
codegenerator/cli/npm/envio/src/Internal.res (1)
200-200: LGTM! Clean addition of call tracking field.The mutable
callsCountfield is appropriately added to the effect type for tracking call metrics.codegenerator/cli/templates/static/codegen/src/LoadLayer.res (1)
99-100: Logic is correct but consider thread safety implications.The increment logic properly tracks the number of effect handler calls by adding the batch size. However, since
callsCountis mutable, consider potential race conditions if effects are called concurrently.Consider whether synchronization is needed for the mutable
callsCountfield in concurrent scenarios.codegenerator/cli/npm/envio/src/Envio.res (2)
42-42: LGTM! Proper metric initialization.Correctly initializes the Prometheus metric to zero when creating an effect.
50-50: LGTM! Consistent field initialization.Properly initializes the
callsCountfield to zero, maintaining consistency with the Prometheus metric.codegenerator/cli/npm/envio/src/Prometheus.res (3)
449-449: Good refactoring for naming consistency.Renaming
countertogaugeimproves code clarity and consistency with the actual metric type being used.
459-459: Consistent variable usage after renaming.Correctly uses the renamed
gaugevariable in the increment function.
578-580: Well-structured label schema definition.The effect labels schema correctly defines the expected structure for Prometheus labels.
| module EffectCallsCount = { | ||
| let gauge = SafeGauge.makeOrThrow( | ||
| ~name="envio_effect_calls_count", | ||
| ~help="The number of calls to the effect. Including both handler execution and cache hits.", | ||
| ~labelSchema=effectLabelsSchema, | ||
| ) | ||
|
|
||
| let set = (~callsCount, ~effectName) => { | ||
| gauge->SafeGauge.handleInt(~labels=effectName, ~value=callsCount) | ||
| } | ||
| } |
There was a problem hiding this comment.
Fix the labels parameter type mismatch.
The set function has a type mismatch - it passes effectName directly as labels, but the schema expects an object with an "effect" field.
Apply this fix:
let set = (~callsCount, ~effectName) => {
- gauge->SafeGauge.handleInt(~labels=effectName, ~value=callsCount)
+ gauge->SafeGauge.handleInt(~labels={effect: effectName}, ~value=callsCount)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| module EffectCallsCount = { | |
| let gauge = SafeGauge.makeOrThrow( | |
| ~name="envio_effect_calls_count", | |
| ~help="The number of calls to the effect. Including both handler execution and cache hits.", | |
| ~labelSchema=effectLabelsSchema, | |
| ) | |
| let set = (~callsCount, ~effectName) => { | |
| gauge->SafeGauge.handleInt(~labels=effectName, ~value=callsCount) | |
| } | |
| } | |
| module EffectCallsCount = { | |
| let gauge = SafeGauge.makeOrThrow( | |
| ~name="envio_effect_calls_count", | |
| ~help="The number of calls to the effect. Including both handler execution and cache hits.", | |
| ~labelSchema=effectLabelsSchema, | |
| ) | |
| let set = (~callsCount, ~effectName) => { | |
| gauge->SafeGauge.handleInt(~labels={effect: effectName}, ~value=callsCount) | |
| } | |
| } |
🤖 Prompt for AI Agents
In codegenerator/cli/npm/envio/src/Prometheus.res around lines 582 to 592, the
set function incorrectly passes effectName directly as labels to
SafeGauge.handleInt, causing a type mismatch since the label schema expects an
object with an "effect" field. Fix this by wrapping effectName in an object with
the key "effect" when passing it as the labels argument, ensuring it matches the
expected label schema.
| options: effectOptions<'input, 'output>, | ||
| handler: effectArgs<'input> => promise<'output>, | ||
| ) => { | ||
| Prometheus.EffectCallsCount.set(~callsCount=0, ~effectName=options.name) |
There was a problem hiding this comment.
Should be fine but is 32 bit int big enough?
Related to https://github.com/enviodev/ui/pull/584
Summary by CodeRabbit