feat(protocols): add duration fields to other events#7428
Open
dwisiswant0 wants to merge 3 commits into
Open
Conversation
Add matcher and extractor duration data[1] for
DNS, SSL, WHOIS, TCP, WebSocket, and Headless
requests.
Track step durations for TCP inputs, WebSocket
message exchanges, and Headless actions. The
`duration` field holds the most recently measured
operation.
Expose request ID-prefixed duration values through
the existing template context path, and add
protocol tests for duration extraction, aliases,
and request ID handling.
[1]: The data exposed is `duration`, `duration_N`,
`<request-id>_duration`, and
`<request-id>_duration_<N>`.
Closes #5413
Closes #7422
Signed-off-by: Dwi Siswanto <git@dw1.io>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds runtime duration measurement across DNS, Network, Headless, SSL, WebSocket, and WHOIS protocols, emits ChangesDuration Support Across Protocols
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pkg/protocols/dns/request.go (1)
164-190:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDuration currently includes optional trace time
durationis calculated after the trace branch, so withtrace: trueit includes trace recursion time in addition to the DNS query. That skewsdurationmatchers/extractors for this protocol operation.Suggested fix
// Send the request to the target servers timeStart := time.Now() response, err := dnsClient.Do(compiledRequest) + duration := time.Since(timeStart) if err != nil { request.options.Output.Request(request.options.TemplatePath, domain, request.Type().String(), err) request.options.Progress.IncrementFailedRequestsBy(1) } else { request.options.Progress.IncrementRequests() } @@ - duration := time.Since(timeStart) - // Create the output event outputEvent := request.responseToDSLMap(compiledRequest, response, domain, question, traceData, duration)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/protocols/dns/request.go` around lines 164 - 190, The duration for the DNS query is measured after the optional trace, so when request.Trace is true the duration includes trace recursion time; fix this by measuring the query duration immediately after the dnsClient.Do handling (capture queryDuration := time.Since(timeStart) right after response is validated and before entering the trace block), then perform the trace (and if you need a separate trace duration use a new timer) and finally pass queryDuration into request.responseToDSLMap (instead of a duration that includes trace). Update references to timeStart/duration accordingly in request.requestToDSLMap call and leave trace timing isolated in request.dnsClient.Trace handling.pkg/protocols/network/request.go (1)
356-385:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
durationis under-measured for write-only steps and may be missingWhen
input.Read == 0, duration is captured right afterconn.Write, but the actual response read happens later at the final read path. That makesdurationeffectively write-latency for common write-then-read flows, and can leave no duration fields for some paths.Suggested direction
+ var pendingStepStart time.Time + hasPendingStep := false timeStart := time.Now() if _, err := conn.Write(dataInBytes); err != nil { ... } if input.Read > 0 { buffer, err := ConnReadNWithTimeout(...) if err != nil { ... } stepDurations = append(stepDurations, time.Since(timeStart)) + hasPendingStep = false } else { - stepDurations = append(stepDurations, time.Since(timeStart)) + pendingStepStart = timeStart + hasPendingStep = true } + finalReadStart := time.Now() + if hasPendingStep { + finalReadStart = pendingStepStart + } final, readErr := ConnReadNWithTimeout(...) ... + if hasPendingStep || len(stepDurations) == 0 { + stepDurations = append(stepDurations, time.Since(finalReadStart)) + }Also applies to: 415-425, 466-471
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/protocols/network/request.go` around lines 356 - 385, The timing for request steps is being recorded too early (timeStart is measured before conn.Write and appended immediately when input.Read == 0), causing write-only steps to record only write latency or miss duration entirely; change the logic so that timeStart is set before the full send/read cycle for a given input and that stepDurations is appended only after the actual response read completes (i.e., move or delay the append to the branch where ConnReadNWithTimeout (and other final-read paths) finish), ensuring symbols like timeStart, conn.Write, ConnReadNWithTimeout, responseBuilder, input.Read, stepDurations and the post-write read branches all use the same start/stop pair so durations reflect the full request/response roundtrip.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/tests/integration/network_test.go`:
- Around line 130-142: The handler in the duration subtest must fail fast on
unexpected payloads: after reading with reader.ConnReadNWithTimeout, verify the
payloads exactly match "FIRST" and "SECOND" and if not call errState.Set with a
descriptive error and return immediately (instead of silently continuing);
update the branches around the first check (where you currently write "PING")
and the second check (where you write "PONG" and then "NUCLEI") to set
errState.Set(fmt.Errorf(...)) and return on mismatch so the subtest fails when
request sequencing/content is wrong, keeping use of conn and
reader.ConnReadNWithTimeout intact.
---
Outside diff comments:
In `@pkg/protocols/dns/request.go`:
- Around line 164-190: The duration for the DNS query is measured after the
optional trace, so when request.Trace is true the duration includes trace
recursion time; fix this by measuring the query duration immediately after the
dnsClient.Do handling (capture queryDuration := time.Since(timeStart) right
after response is validated and before entering the trace block), then perform
the trace (and if you need a separate trace duration use a new timer) and
finally pass queryDuration into request.responseToDSLMap (instead of a duration
that includes trace). Update references to timeStart/duration accordingly in
request.requestToDSLMap call and leave trace timing isolated in
request.dnsClient.Trace handling.
In `@pkg/protocols/network/request.go`:
- Around line 356-385: The timing for request steps is being recorded too early
(timeStart is measured before conn.Write and appended immediately when
input.Read == 0), causing write-only steps to record only write latency or miss
duration entirely; change the logic so that timeStart is set before the full
send/read cycle for a given input and that stepDurations is appended only after
the actual response read completes (i.e., move or delay the append to the branch
where ConnReadNWithTimeout (and other final-read paths) finish), ensuring
symbols like timeStart, conn.Write, ConnReadNWithTimeout, responseBuilder,
input.Read, stepDurations and the post-write read branches all use the same
start/stop pair so durations reflect the full request/response roundtrip.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ac4f0c48-e9b7-4958-b3e8-f005563a7633
⛔ Files ignored due to path filters (6)
internal/tests/integration/testdata/protocols/dns/duration.yamlis excluded by!**/*.yamlinternal/tests/integration/testdata/protocols/headless/headless-duration.yamlis excluded by!**/*.yamlinternal/tests/integration/testdata/protocols/network/duration.yamlis excluded by!**/*.yamlinternal/tests/integration/testdata/protocols/ssl/duration.yamlis excluded by!**/*.yamlinternal/tests/integration/testdata/protocols/websocket/duration.yamlis excluded by!**/*.yamlinternal/tests/integration/testdata/protocols/whois/duration.yamlis excluded by!**/*.yaml
📒 Files selected for processing (29)
SYNTAX-REFERENCE.mdinternal/tests/integration/dns_test.gointernal/tests/integration/headless_test.gointernal/tests/integration/network_test.gointernal/tests/integration/ssl_test.gointernal/tests/integration/websocket_test.gointernal/tests/integration/whois_test.gopkg/protocols/dns/dns.gopkg/protocols/dns/operators.gopkg/protocols/dns/operators_test.gopkg/protocols/dns/request.gopkg/protocols/headless/engine/page.gopkg/protocols/headless/engine/page_actions.gopkg/protocols/headless/engine/page_actions_test.gopkg/protocols/headless/headless.gopkg/protocols/headless/operators_test.gopkg/protocols/headless/request.gopkg/protocols/http/request_condition_test.gopkg/protocols/network/network.gopkg/protocols/network/operators_test.gopkg/protocols/network/request.gopkg/protocols/network/request_duration_test.gopkg/protocols/ssl/ssl.gopkg/protocols/ssl/ssl_test.gopkg/protocols/websocket/websocket.gopkg/protocols/websocket/websocket_test.gopkg/protocols/whois/whois.gopkg/protocols/whois/whois_test.gopkg/templates/templates_doc.go
Comment on lines
+130
to
+142
| if string(data) == "FIRST" { | ||
| _, _ = conn.Write([]byte("PING")) | ||
| } | ||
|
|
||
| data, err = reader.ConnReadNWithTimeout(conn, 6, 5*time.Second) | ||
| if err != nil { | ||
| errState.Set(err) | ||
| return | ||
| } | ||
| if string(data) == "SECOND" { | ||
| _, _ = conn.Write([]byte("PONG")) | ||
| } | ||
| _, _ = conn.Write([]byte("NUCLEI")) |
Contributor
There was a problem hiding this comment.
Fail fast on unexpected step payloads in the duration subtest.
This handler only writes on matching inputs but doesn’t mark mismatches as failures, so the subtest may still pass with wrong request sequencing/content.
Suggested fix
- if string(data) == "FIRST" {
- _, _ = conn.Write([]byte("PING"))
- }
+ if string(data) == "FIRST" {
+ _, _ = conn.Write([]byte("PING"))
+ } else {
+ errState.Set(fmt.Errorf("invalid first data received: %s", string(data)))
+ return
+ }
@@
- if string(data) == "SECOND" {
- _, _ = conn.Write([]byte("PONG"))
- }
+ if string(data) == "SECOND" {
+ _, _ = conn.Write([]byte("PONG"))
+ } else {
+ errState.Set(fmt.Errorf("invalid second data received: %s", string(data)))
+ return
+ }📝 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.
Suggested change
| if string(data) == "FIRST" { | |
| _, _ = conn.Write([]byte("PING")) | |
| } | |
| data, err = reader.ConnReadNWithTimeout(conn, 6, 5*time.Second) | |
| if err != nil { | |
| errState.Set(err) | |
| return | |
| } | |
| if string(data) == "SECOND" { | |
| _, _ = conn.Write([]byte("PONG")) | |
| } | |
| _, _ = conn.Write([]byte("NUCLEI")) | |
| if string(data) == "FIRST" { | |
| _, _ = conn.Write([]byte("PING")) | |
| } else { | |
| errState.Set(fmt.Errorf("invalid first data received: %s", string(data))) | |
| return | |
| } | |
| data, err = reader.ConnReadNWithTimeout(conn, 6, 5*time.Second) | |
| if err != nil { | |
| errState.Set(err) | |
| return | |
| } | |
| if string(data) == "SECOND" { | |
| _, _ = conn.Write([]byte("PONG")) | |
| } else { | |
| errState.Set(fmt.Errorf("invalid second data received: %s", string(data))) | |
| return | |
| } | |
| _, _ = conn.Write([]byte("NUCLEI")) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/tests/integration/network_test.go` around lines 130 - 142, The
handler in the duration subtest must fail fast on unexpected payloads: after
reading with reader.ConnReadNWithTimeout, verify the payloads exactly match
"FIRST" and "SECOND" and if not call errState.Set with a descriptive error and
return immediately (instead of silently continuing); update the branches around
the first check (where you currently write "PING") and the second check (where
you write "PONG" and then "NUCLEI") to set errState.Set(fmt.Errorf(...)) and
return on mismatch so the subtest fails when request sequencing/content is
wrong, keeping use of conn and reader.ConnReadNWithTimeout intact.
Signed-off-by: Dwi Siswanto <git@dw1.io>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/protocols/websocket/websocket_test.go`:
- Around line 289-295: The test handler is discarding errors from
wsutil.WriteServerMessage in the "hello" and "status" branches; capture the
returned error from wsutil.WriteServerMessage (in the cases handling "hello" and
"status") and if non-nil call the test failure helper (e.g., t.Fatalf or
t.Errorf and return) with a clear message including the operation and error;
ensure you reference conn, op and durationObservationDelay in the error context
so write failures fail fast and show actionable diagnostics.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b8e6d385-fe17-49bc-92e7-c2c6ba104243
📒 Files selected for processing (2)
pkg/protocols/network/request_duration_test.gopkg/protocols/websocket/websocket_test.go
Comment on lines
+289
to
+295
| case "hello": | ||
| time.Sleep(durationObservationDelay) | ||
| _ = wsutil.WriteServerMessage(conn, op, []byte("world")) | ||
| case "status": | ||
| time.Sleep(durationObservationDelay) | ||
| _ = wsutil.WriteServerMessage(conn, op, []byte("ready")) | ||
| default: |
Contributor
There was a problem hiding this comment.
Handle websocket write errors in the test handler.
On Line 291 and Line 294, write errors are discarded. If a server write fails, the test can fail later with less actionable symptoms.
Suggested fix
switch string(msg) {
case "hello":
time.Sleep(durationObservationDelay)
- _ = wsutil.WriteServerMessage(conn, op, []byte("world"))
+ if err := wsutil.WriteServerMessage(conn, op, []byte("world")); err != nil {
+ return
+ }
case "status":
time.Sleep(durationObservationDelay)
- _ = wsutil.WriteServerMessage(conn, op, []byte("ready"))
+ if err := wsutil.WriteServerMessage(conn, op, []byte("ready")); err != nil {
+ return
+ }
default:
return
}📝 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.
Suggested change
| case "hello": | |
| time.Sleep(durationObservationDelay) | |
| _ = wsutil.WriteServerMessage(conn, op, []byte("world")) | |
| case "status": | |
| time.Sleep(durationObservationDelay) | |
| _ = wsutil.WriteServerMessage(conn, op, []byte("ready")) | |
| default: | |
| case "hello": | |
| time.Sleep(durationObservationDelay) | |
| if err := wsutil.WriteServerMessage(conn, op, []byte("world")); err != nil { | |
| return | |
| } | |
| case "status": | |
| time.Sleep(durationObservationDelay) | |
| if err := wsutil.WriteServerMessage(conn, op, []byte("ready")); err != nil { | |
| return | |
| } | |
| default: |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/protocols/websocket/websocket_test.go` around lines 289 - 295, The test
handler is discarding errors from wsutil.WriteServerMessage in the "hello" and
"status" branches; capture the returned error from wsutil.WriteServerMessage (in
the cases handling "hello" and "status") and if non-nil call the test failure
helper (e.g., t.Fatalf or t.Errorf and return) with a clear message including
the operation and error; ensure you reference conn, op and
durationObservationDelay in the error context so write failures fail fast and
show actionable diagnostics.
Signed-off-by: Dwi Siswanto <git@dw1.io>
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
1 participant
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.
Proposed changes
Add matcher and extractor duration data[1] for
DNS, SSL, WHOIS, TCP, WebSocket, and Headless
requests.
Track step durations for TCP inputs, WebSocket
message exchanges, and Headless actions. The
durationfield holds the most recently measuredoperation.
Expose request ID-prefixed duration values through
the existing template context path, and add
protocol tests for duration extraction, aliases,
and request ID handling.
[1]: The data exposed is
duration,duration_N,<request-id>_duration, and<request-id>_duration_<N>.Closes #5413
Closes #7422
Proof
Checklist
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation