Skip to content

Commit

Permalink
Remove tracing dependency from http-lib and add baggage (#6065)
Browse files Browse the repository at this point in the history
Removes dependency on `tracing` from `http-lib` and add baggage.

---

The introduction of it as a dependency was to support this style of
code:

```ocaml
let@ req = Http.Request.with_tracing ~name req in
```

However, HTTP headers are just a carrier in the context of tracing. The
library ought to be as inert as possible; providing enough to endow
requests with tracing information, but not calling into the tracing
library to export child spans (as is done above).

To this end, the notion of a
[propagator](https://opentelemetry.io/docs/specs/otel/context/api-propagators/)
is introduced that provides an interface for doing the above, but
generalised to arbitrary carriers. If you can describe how to
inject/extract a "trace context" into/from a carrier, you can
conceivably propagate tracing across it.

---

In future, the idea will be to:
- This trace context can be inherited by spans, so it can reach the
exporter. We can probably use the `SpanContext.t` record to do this. If
this information can reach the exporter, we can choose to decorate spans
with tags derived from the trace context (such as baggage) during
exportation.
- At incoming service boundaries, instead of pushing down a `Span.t
option` derived from an encoded `traceparent` in the carrier, we will
push down `TraceContext` (which is more general than the encoded
`SpanContext` carried by the optional `Span.t` that's currently pushed
down). This is so we can more easily handle the case where baggage is
provided but a traceparent isn't: in which case, a span is still created
if tracing is enabled, but with no parent to inherit contextual metadata
from (a previous - abandoned - PR introduced baggage in a way that only
worked when a traceparent was provided).

---

Opening this as a draft for now. Would appreciate any commentary. There
appears to be duplication of `Helper` but this is largely because XAPI's
`dune` defines separate libraries which have slightly different
dependencies.

This has been manually tested with Jaeger, but more thorough testing
should probably be done (comparing deeper span forests before and after
these changes).
  • Loading branch information
robhoes authored Nov 26, 2024
2 parents 2994fcd + c5fe9ba commit 6f64a78
Show file tree
Hide file tree
Showing 23 changed files with 470 additions and 126 deletions.
2 changes: 1 addition & 1 deletion ocaml/libs/http-lib/dune
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,6 @@
xapi-stdext-threads
xapi-stdext-unix
xml-light2
tracing
)
)

Expand All @@ -46,6 +45,7 @@
polly
threads.posix
tracing
tracing_propagator
uri
xapi-log
xapi-stdext-pervasives
Expand Down
40 changes: 2 additions & 38 deletions ocaml/libs/http-lib/http.ml
Original file line number Diff line number Diff line change
Expand Up @@ -132,8 +132,6 @@ module Hdr = struct

let location = "location"

let traceparent = "traceparent"

let hsts = "strict-transport-security"
end

Expand Down Expand Up @@ -522,7 +520,6 @@ module Request = struct
; mutable close: bool
; additional_headers: (string * string) list
; body: string option
; traceparent: string option
}
[@@deriving rpc]

Expand All @@ -546,12 +543,11 @@ module Request = struct
; close= true
; additional_headers= []
; body= None
; traceparent= None
}

let make ?(frame = false) ?(version = "1.1") ?(keep_alive = true) ?accept
?cookie ?length ?auth ?subtask_of ?body ?(headers = []) ?content_type
?host ?(query = []) ?traceparent ~user_agent meth path =
?host ?(query = []) ~user_agent meth path =
{
empty with
version
Expand All @@ -570,7 +566,6 @@ module Request = struct
; body
; accept
; query
; traceparent
}

let get_version x = x.version
Expand All @@ -582,8 +577,7 @@ module Request = struct
Printf.sprintf
"{ frame = %b; method = %s; uri = %s; query = [ %s ]; content_length = [ \
%s ]; transfer encoding = %s; version = %s; cookie = [ %s ]; task = %s; \
subtask_of = %s; content-type = %s; host = %s; user_agent = %s; \
traceparent = %s }"
subtask_of = %s; content-type = %s; host = %s; user_agent = %s; }"
x.frame (string_of_method_t x.m) x.uri (kvpairs x.query)
(Option.fold ~none:"" ~some:Int64.to_string x.content_length)
(Option.value ~default:"" x.transfer_encoding)
Expand All @@ -593,7 +587,6 @@ module Request = struct
(Option.value ~default:"" x.content_type)
(Option.value ~default:"" x.host)
(Option.value ~default:"" x.user_agent)
(Option.value ~default:"" x.traceparent)

let to_header_list x =
let kvpairs x =
Expand Down Expand Up @@ -643,11 +636,6 @@ module Request = struct
~some:(fun x -> [Hdr.user_agent ^ ": " ^ x])
x.user_agent
in
let traceparent =
Option.fold ~none:[]
~some:(fun x -> [Hdr.traceparent ^ ": " ^ x])
x.traceparent
in
let close =
[(Hdr.connection ^ ": " ^ if x.close then "close" else "keep-alive")]
in
Expand All @@ -665,7 +653,6 @@ module Request = struct
@ content_type
@ host
@ user_agent
@ traceparent
@ close
@ List.map (fun (k, v) -> k ^ ": " ^ v) x.additional_headers

Expand All @@ -687,29 +674,6 @@ module Request = struct
let headers, body = to_headers_and_body x in
let frame_header = if x.frame then make_frame_header headers else "" in
frame_header ^ headers ^ body

let traceparent_of req =
let open Tracing in
let ( let* ) = Option.bind in
let* traceparent = req.traceparent in
let* span_context = SpanContext.of_traceparent traceparent in
let span = Tracer.span_of_span_context span_context req.uri in
Some span

let with_tracing ?attributes ~name req f =
let open Tracing in
let parent = traceparent_of req in
with_child_trace ?attributes parent ~name (fun (span : Span.t option) ->
match span with
| Some span ->
let traceparent =
Some (span |> Span.get_context |> SpanContext.to_traceparent)
in
let req = {req with traceparent} in
f req
| None ->
f req
)
end

module Response = struct
Expand Down
9 changes: 0 additions & 9 deletions ocaml/libs/http-lib/http.mli
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,6 @@ module Request : sig
; mutable close: bool
; additional_headers: (string * string) list
; body: string option
; traceparent: string option
}

val rpc_of_t : t -> Rpc.t
Expand All @@ -109,7 +108,6 @@ module Request : sig
-> ?content_type:string
-> ?host:string
-> ?query:(string * string) list
-> ?traceparent:string
-> user_agent:string
-> method_t
-> string
Expand All @@ -128,11 +126,6 @@ module Request : sig

val to_wire_string : t -> string
(** [to_wire_string t] returns a string which could be sent to a server *)

val traceparent_of : t -> Tracing.Span.t option

val with_tracing :
?attributes:(string * string) list -> name:string -> t -> (t -> 'a) -> 'a
end

(** Parsed form of the HTTP response *)
Expand Down Expand Up @@ -229,8 +222,6 @@ module Hdr : sig

val location : string

val traceparent : string

val hsts : string
(** Header used for HTTP Strict Transport Security *)
end
Expand Down
33 changes: 23 additions & 10 deletions ocaml/libs/http-lib/http_svr.ml
Original file line number Diff line number Diff line change
Expand Up @@ -99,9 +99,17 @@ let response_of_request req hdrs =
~headers:(connection :: cache :: hdrs)
"200" "OK"

module Helper = struct
include Tracing.Propagator.Make (struct
include Tracing_propagator.Propagator.Http

let name_span req = req.Http.Request.uri
end)
end

let response_fct req ?(hdrs = []) s (response_length : int64)
(write_response_to_fd_fn : Unix.file_descr -> unit) =
let@ req = Http.Request.with_tracing ~name:__FUNCTION__ req in
let@ req = Helper.with_tracing ~name:__FUNCTION__ req in
let res =
{
(response_of_request req hdrs) with
Expand Down Expand Up @@ -409,8 +417,6 @@ let read_request_exn ~proxy_seen ~read_timeout ~total_timeout ~max_length fd =
{req with host= Some v}
| k when k = Http.Hdr.user_agent ->
{req with user_agent= Some v}
| k when k = Http.Hdr.traceparent ->
{req with traceparent= Some v}
| k when k = Http.Hdr.connection && lowercase v = "close" ->
{req with close= true}
| k
Expand All @@ -436,18 +442,25 @@ let read_request_exn ~proxy_seen ~read_timeout ~total_timeout ~max_length fd =
already sent back a suitable error code and response to the client. *)
let read_request ?proxy_seen ~read_timeout ~total_timeout ~max_length fd =
try
(* TODO: Restore functionality of tracing this function. We rely on the request
to contain information we want spans to inherit. However, it is the reading of the
request that we intend to trace. *)
let r, proxy =
read_request_exn ~proxy_seen ~read_timeout ~total_timeout ~max_length fd
in
let trace_context = Tracing_propagator.Propagator.Http.extract_from r in
let tracer = Tracing.Tracer.get_tracer ~name:"http_tracer" in
let loop_span =
match Tracing.Tracer.start ~tracer ~name:__FUNCTION__ ~parent:None () with
match
Tracing.Tracer.start ~tracer ~trace_context ~name:__FUNCTION__
~parent:None ()
with
| Ok span ->
span
| Error _ ->
None
in
let r, proxy =
read_request_exn ~proxy_seen ~read_timeout ~total_timeout ~max_length fd
in
let parent_span = Http.Request.traceparent_of r in
let parent_span = Helper.traceparent_of r in
let loop_span =
Option.fold ~none:None
~some:(fun span ->
Expand Down Expand Up @@ -491,8 +504,8 @@ let read_request ?proxy_seen ~read_timeout ~total_timeout ~max_length fd =
(None, None)

let handle_one (x : 'a Server.t) ss context req =
let@ req = Http.Request.with_tracing ~name:__FUNCTION__ req in
let span = Http.Request.traceparent_of req in
let@ req = Helper.with_tracing ~name:__FUNCTION__ req in
let span = Helper.traceparent_of req in
let finished = ref false in
try
D.debug "Request %s" (Http.Request.to_string req) ;
Expand Down
10 changes: 2 additions & 8 deletions ocaml/libs/http-lib/xmlrpc_client.ml
Original file line number Diff line number Diff line change
Expand Up @@ -49,16 +49,10 @@ let connect ?session_id ?task_id ?subtask_of path =
?subtask_of Http.Connect path

let xmlrpc ?frame ?version ?keep_alive ?task_id ?cookie ?length ?auth
?subtask_of ?query ?body ?(tracing = None) path =
let traceparent =
let open Tracing in
Option.map
(fun span -> Span.get_context span |> SpanContext.to_traceparent)
tracing
in
?subtask_of ?query ?body path =
let headers = Option.map (fun x -> [(Http.Hdr.task_id, x)]) task_id in
Http.Request.make ~user_agent ?frame ?version ?keep_alive ?cookie ?headers
?length ?auth ?subtask_of ?query ?body ?traceparent Http.Post path
?length ?auth ?subtask_of ?query ?body Http.Post path

(** Thrown when ECONNRESET is caught which suggests the remote crashed or restarted *)
exception Connection_reset
Expand Down
1 change: 0 additions & 1 deletion ocaml/libs/http-lib/xmlrpc_client.mli
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,6 @@ val xmlrpc :
-> ?subtask_of:string
-> ?query:(string * string) list
-> ?body:string
-> ?tracing:Tracing.Span.t option
-> string
-> Http.Request.t
(** Returns an HTTP.Request.t representing an XMLRPC request *)
Expand Down
5 changes: 5 additions & 0 deletions ocaml/libs/tracing/dune
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,11 @@
(preprocess
(pps ppx_deriving_rpc)))

(library
(name tracing_propagator)
(modules propagator)
(libraries astring http-lib tracing))

(test
(name test_tracing)
(modules test_tracing)
Expand Down
109 changes: 109 additions & 0 deletions ocaml/libs/tracing/propagator.ml
Original file line number Diff line number Diff line change
@@ -0,0 +1,109 @@
(*
* Copyright (c) Cloud Software Group, Inc.
*
* This program is free software; you can redistribute it and/or modify
* it under the terms of the GNU Lesser General Public License as published
* by the Free Software Foundation; version 2.1 only. with the special
* exception on linking described in file LICENSE.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU Lesser General Public License for more details.
*)

module type S = sig
type carrier

val inject_into : Tracing.TraceContext.t -> carrier -> carrier

val extract_from : carrier -> Tracing.TraceContext.t
end

let ( let* ) = Option.bind

let ( >> ) f g x = g (f x)

let maybe f = function Some _ as o -> f o | _ -> Fun.id

let[@tail_mod_cons] rec filter_append p xs ys =
match xs with
| [] ->
ys
| x :: xs when p x ->
x :: filter_append p xs ys
| _ :: xs ->
filter_append p xs ys

module Http = struct
type carrier = Http.Request.t

open struct
let hdr_traceparent = "traceparent"

let hdr_baggage = "baggage"
end

let alloc_assoc k kvs =
List.filter_map
(fun (key, value) -> if key = k then Some value else None)
kvs
|> function
| [] ->
None
| xs ->
Some xs

let parse input =
let open Astring.String in
let trim_pair (key, value) = (trim key, trim value) in
input
|> cuts ~sep:";"
|> List.map (cut ~sep:"=" >> Option.map trim_pair)
|> List.filter_map Fun.id

let inject_into ctx req =
let open Tracing in
let traceparent = (hdr_traceparent, TraceContext.traceparent_of ctx) in
let baggage =
let encoded =
let encode =
List.map (fun (k, v) -> Printf.sprintf "%s=%s" k v)
>> String.concat ";"
in
TraceContext.baggage_of ctx |> Option.map encode
in
(hdr_baggage, encoded)
in
let entries = [traceparent; baggage] in
let filter_entries entries =
let tbl = Hashtbl.create 47 in
let record (k, v) =
match v with
| Some v ->
Hashtbl.replace tbl k () ;
Some (k, v)
| _ ->
None
in
let entries = List.filter_map record entries in
(entries, fst >> Hashtbl.mem tbl)
in
let entries, to_replace = filter_entries entries in
let headers = req.Http.Request.additional_headers in
let additional_headers =
filter_append (Fun.negate to_replace) headers entries
in
{req with additional_headers}

let extract_from req =
let open Tracing in
let headers = req.Http.Request.additional_headers in
let traceparent = List.assoc_opt hdr_traceparent headers in
let baggage =
let* all = alloc_assoc hdr_baggage headers in
Some (List.concat_map parse all)
in
let open TraceContext in
empty |> maybe with_traceparent traceparent |> maybe with_baggage baggage
end
Loading

0 comments on commit 6f64a78

Please sign in to comment.