Expose elapsed_ms in step.http_call output for latency measurement#383
Merged
Expose elapsed_ms in step.http_call output for latency measurement#383
Conversation
Co-authored-by: intel352 <77607+intel352@users.noreply.github.com> Agent-Logs-Url: https://github.com/GoCodeAlone/workflow/sessions/e8305985-82c3-4d25-b099-2808597e0880
Copilot
AI
changed the title
[WIP] Add elapsed_ms field to step.http_call output for latency measurement
Expose elapsed_ms in step.http_call output for latency measurement
Mar 26, 2026
Contributor
There was a problem hiding this comment.
Pull request overview
Adds request-duration visibility to the step.http_call pipeline step so workflows can record upstream latency without external timers/workarounds.
Changes:
- Measure request wall-clock duration around
httpClient.Do+io.ReadAll, and expose it aselapsed_msin the step output (including the OAuth2 401 retry path). - Update the builtin step schema to declare
elapsed_msas an output field for validation/tooling. - Add/adjust unit tests to assert
elapsed_msis present and included in inferred outputs.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
module/pipeline_step_http_call.go |
Adds timing measurements and returns elapsed_ms in outputs for normal and retry execution paths. |
module/pipeline_step_http_call_test.go |
Adds a new test ensuring elapsed_ms exists and is non-negative. |
schema/step_schema_builtins.go |
Declares elapsed_ms as a static output on step.http_call for schema-based tooling. |
schema/step_schema_test.go |
Updates fallback output inference test to include elapsed_ms. |
Comment on lines
84
to
89
| Outputs: []StepOutputDef{ | ||
| {Key: "status", Type: "number", Description: "HTTP response status code"}, | ||
| {Key: "body", Type: "any", Description: "Response body (parsed as JSON if Content-Type is application/json)"}, | ||
| {Key: "headers", Type: "map", Description: "Response headers"}, | ||
| {Key: "elapsed_ms", Type: "number", Description: "Request duration in milliseconds (wall-clock time from send to response fully read)"}, | ||
| }, |
There was a problem hiding this comment.
The declared outputs for step.http_call don’t match the actual runtime output from parseHTTPResponse: runtime includes status_code (int) and status (string like "200 OK"), but the schema currently declares status as a numeric status code and does not declare status_code at all. This will cause template validation/UI tooling to mis-type status and potentially warn/error on valid status_code references; consider updating the schema outputs to include status_code (number) and status (string), alongside body, headers, and the new elapsed_ms.
Comment on lines
523
to
531
| respBody, err = io.ReadAll(retryResp.Body) | ||
| retryElapsedMS := time.Since(retryStart).Milliseconds() | ||
| if err != nil { | ||
| return nil, fmt.Errorf("http_call step %q: failed to read retry response: %w", s.name, err) | ||
| } | ||
|
|
||
| output := parseHTTPResponse(retryResp, respBody) | ||
| output["elapsed_ms"] = retryElapsedMS | ||
| if instanceURL := s.oauthEntry.getInstanceURL(); instanceURL != "" { |
There was a problem hiding this comment.
The retry-on-401 path now sets elapsed_ms, but there’s no assertion covering this behavior. Since there’s already a 401 refresh test covering the retry flow, consider extending it (or adding a dedicated test) to assert elapsed_ms is present and non-negative on the retry output as well, so this key doesn’t regress on the refresh path.
⏱ Benchmark Results✅ No significant performance regressions detected. benchstat comparison (baseline → PR)
|
Contributor
|
@copilot apply changes based on the comments in this thread |
…back Co-authored-by: intel352 <77607+intel352@users.noreply.github.com> Agent-Logs-Url: https://github.com/GoCodeAlone/workflow/sessions/c86221d6-c717-44a1-9cb1-a4d24a72dbe9
Contributor
Author
Done in 7ff8266:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
step.http_callhad no way to surface request duration, forcing workarounds (e.g.-1sentinel values) in latency metrics for shadow mode / proxy patterns.Changes
module/pipeline_step_http_call.go— WrapshttpClient.Do+io.ReadAllin atime.Now()/time.Sincemeasurement; addselapsed_ms(int64, ms) to the output map on both the normal and OAuth2 token-refresh retry paths.schema/step_schema_builtins.go— Declareselapsed_msas a static output field onstep.http_callso template validation and UI tooling recognize it.TestHTTPCallStep_ElapsedMSasserts the field is present and non-negative;TestInferStepOutputs_Fallbackupdated to includeelapsed_ms.Usage
Original prompt
🔒 GitHub Advanced Security automatically protects Copilot coding agent pull requests. You can protect all pull requests by enabling Advanced Security for your repositories. Learn more about Advanced Security.