From 37db46759feae0330df116ad9144563ef980bd1d Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Thu, 8 Feb 2024 08:05:53 +0100 Subject: [PATCH] doc(webconnectivitylte): clarify http_transaction_{start,end} semantics (#1495) Part of https://github.com/ooni/probe/issues/2669 --- .../experiment/webconnectivitylte/cleartextflow.go | 13 +++++++------ .../experiment/webconnectivitylte/secureflow.go | 13 +++++++------ 2 files changed, 14 insertions(+), 12 deletions(-) diff --git a/internal/experiment/webconnectivitylte/cleartextflow.go b/internal/experiment/webconnectivitylte/cleartextflow.go index c9814b4bd..90af52866 100644 --- a/internal/experiment/webconnectivitylte/cleartextflow.go +++ b/internal/experiment/webconnectivitylte/cleartextflow.go @@ -243,15 +243,13 @@ func (t *CleartextFlow) httpTransaction(ctx context.Context, network, address, a txp model.HTTPTransport, req *http.Request, trace *measurexlite.Trace) (*http.Response, []byte, error) { const maxbody = 1 << 19 started := trace.TimeSince(trace.ZeroTime()) - // TODO(bassosimone): I am wondering whether we should have the HTTP transaction - // start at the beginning of the flow rather than here. If we start it at the - // beginning this is nicer, but, at the same time, starting it at the beginning - // of the flow means we're not collecting information about DNS. So, I am a - // bit torn about what is the best approach to follow here. Maybe it does not - // even matter to emit transaction_start/end events given that we have transaction ID. + + // Implementation note: we want to emit http_transaction_start when we actually start doing + // HTTP things such that it's possible to correctly classify network events t.TestKeys.AppendNetworkEvents(measurexlite.NewAnnotationArchivalNetworkEvent( trace.Index(), started, "http_transaction_start", trace.Tags()..., )) + resp, err := txp.RoundTrip(req) var body []byte if err == nil { @@ -265,10 +263,12 @@ func (t *CleartextFlow) httpTransaction(ctx context.Context, network, address, a if err == nil && httpRedirectIsRedirect(resp) { err = httpValidateRedirect(resp) } + finished := trace.TimeSince(trace.ZeroTime()) t.TestKeys.AppendNetworkEvents(measurexlite.NewAnnotationArchivalNetworkEvent( trace.Index(), finished, "http_transaction_done", trace.Tags()..., )) + ev := measurexlite.NewArchivalHTTPRequestResult( trace.Index(), started, @@ -284,6 +284,7 @@ func (t *CleartextFlow) httpTransaction(ctx context.Context, network, address, a finished, trace.Tags()..., ) + t.TestKeys.PrependRequests(ev) return resp, body, err } diff --git a/internal/experiment/webconnectivitylte/secureflow.go b/internal/experiment/webconnectivitylte/secureflow.go index 1d794869c..486b94d24 100644 --- a/internal/experiment/webconnectivitylte/secureflow.go +++ b/internal/experiment/webconnectivitylte/secureflow.go @@ -298,15 +298,13 @@ func (t *SecureFlow) httpTransaction(ctx context.Context, network, address, alpn txp model.HTTPTransport, req *http.Request, trace *measurexlite.Trace) (*http.Response, []byte, error) { const maxbody = 1 << 19 started := trace.TimeSince(trace.ZeroTime()) - // TODO(bassosimone): I am wondering whether we should have the HTTP transaction - // start at the beginning of the flow rather than here. If we start it at the - // beginning this is nicer, but, at the same time, starting it at the beginning - // of the flow means we're not collecting information about DNS. So, I am a - // bit torn about what is the best approach to follow here. Maybe it does not - // even matter to emit transaction_start/end events given that we have transaction ID. + + // Implementation note: we want to emit http_transaction_start when we actually start doing + // HTTP things such that it's possible to correctly classify network events t.TestKeys.AppendNetworkEvents(measurexlite.NewAnnotationArchivalNetworkEvent( trace.Index(), started, "http_transaction_start", trace.Tags()..., )) + resp, err := txp.RoundTrip(req) var body []byte if err == nil { @@ -320,10 +318,12 @@ func (t *SecureFlow) httpTransaction(ctx context.Context, network, address, alpn if err == nil && httpRedirectIsRedirect(resp) { err = httpValidateRedirect(resp) } + finished := trace.TimeSince(trace.ZeroTime()) t.TestKeys.AppendNetworkEvents(measurexlite.NewAnnotationArchivalNetworkEvent( trace.Index(), finished, "http_transaction_done", trace.Tags()..., )) + ev := measurexlite.NewArchivalHTTPRequestResult( trace.Index(), started, @@ -339,6 +339,7 @@ func (t *SecureFlow) httpTransaction(ctx context.Context, network, address, alpn finished, trace.Tags()..., ) + t.TestKeys.PrependRequests(ev) return resp, body, err }