fix(observability): stamp HTTP logs with request-issue time (SDK-6164) - #2
Open
harshit-browserstack wants to merge 1 commit into
Open
fix(observability): stamp HTTP logs with request-issue time (SDK-6164)#2harshit-browserstack wants to merge 1 commit into
harshit-browserstack wants to merge 1 commit into
Conversation
SDK-6164: HTTP logs rendered out of order in the Test Observability Logs tab. createHttpLogEvent stamped the LogCreated event with httpResponse[0], the instant the response landed. Nightwatch pushes each httpOutput entry as [isoTimestamp, message, params] when the line is logged, so httpResponse[0] is request-issue time plus the whole round trip. A slow request therefore carried a later timestamp than logs emitted while it was still in flight and sorted after them. Stamp with httpRequest[0] instead. duration_ms is computed from both entries independently, so it still spans request -> response and no metric changes. This also matches createScreenshotLogEvent, which already stamps with the command's startTime. Adds test/src/test-observability/createHttpLogEvent.js (4 cases).
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.
Summary
createHttpLogEventstamps eachLogCreated/HTTPevent withhttpResponse[0]— the instant the response landed — so HTTP logs render out of order in the Test Observability Logs tab. A request that takes 5s carries a timestamp 5s after it was issued, and therefore sorts after every log line the test wrote while it was still in flight.This PR stamps the event with
httpRequest[0](request-issued) instead. One field;duration_msis unaffected.Linked ticket: BrowserStack SDK-6164
Target package release:
@nightwatch/browserstack@3.11.4(patch — bug fix, no API change)TRA Link
N/A — this is a plugin-side code change, not a BStackAutomation test run, so there is no regression TRA build for it. Jira: https://browserstack.atlassian.net/browse/SDK-6164
Details
Root cause
Nightwatch builds every
httpOutputentry as[isoTimestamp, message, params]at the moment that line is logged:logRequest()runs before the request is sent (lib/http/request.js, right aftercreateHttpRequest());logResponse()runs fromonRequestComplete. SohttpResponse[0]is request-issue time plus the entire round trip.The plugin used that value as the log's own timestamp:
The line dates to the original HTTP-log implementation (
f9827994, Apr 2023) and carries no rationale —duration_mswas added separately later (c39e842d). Nothing else in the plugin reads a log event'stimestamp.Fix
Stamp with
httpRequest[0].duration_msis computed fromhttpRequest[0]andhttpResponse[0]independently, so it still spans request → response. No metric changes.createScreenshotLogEvent, which already stamps with the command'sstartTimerather than a completion time.Scope — what this PR deliberately does NOT include
SDK-6164 also proposes a monotonic
sequencetiebreaker so logs sharing the same millisecond order deterministically. That half is intentionally left out:sequencefield is a TRA backend question. The backend sort logic lives in no SDK-side repo, and the internal read endpoint (api/v1/testRuns/<id>/consolidatedLogs) returns401 Unauthorized Access!with the credentials available here, so the stored/served order could not be read back to confirm it.sequencetoday would be inert until the backend adopts it as a secondary sort key, and the gRPCLogCreatedEventRequest.LogEntryproto has nosequencefield, so the Binary/gRPC flow could not carry it for other SDKs.Tracked as a cross-team follow-up. This PR stands on its own: it makes the HTTP log's timestamp mean what it claims to mean, which is correct regardless of that decision.
How Has This Been Tested?
npm test: 94 passing, 0 failing. Addstest/src/test-observability/createHttpLogEvent.js(4 cases): request-time stamping,duration_msstill 5000ms across the span, issue-order preserved for a slow-then-fast request pair, and the non-Request/Responseguard.BROWSERSTACK_USERNAME/BROWSERSTACK_ACCESS_KEYexported, 12 tests intest/src/utils/helper.jsfail because they assert env-derived getters return literals. Those same 12 fail identically on unmodifiedorigin/main(78 passing / 12 failing vs. 82 / 12 here — i.e. +4 new tests, no new failures). Pre-existing and unrelated.npm run eslint: clean.LogCreatedbatch posted tohttps://collector-observability.browserstack.com/api/v1/batchwas accepted (200 {"success":true}) carrying request-issued timestamps.Screenshots (if appropriate):
N/A — no UI change in this repo. The customer-visible effect is log ordering in the Test Observability Logs tab, captured in the SDK-6164 ticket.
Types of changes
Checklist: