From 30a6ebf7136c62d5487087c9e698bc7d2169098f Mon Sep 17 00:00:00 2001 From: Gabriel Buica Date: Thu, 18 Jan 2024 09:42:31 +0000 Subject: [PATCH 1/2] CP-46631: Improved list of span attributes. Adds: - `xs.xapi.task.id`; - `xs.xapi.task.name`; - `xs.xapi.task.uuid`; - `xs.xapi.task.session.track.id`; - `xs.xapi.task.origin`; to the list of possible attributes of spans created by `context.ml` This improves the debuggability and makes spans more easily identifiable. Signed-off-by: Gabriel Buica --- ocaml/xapi/context.ml | 60 +++++++++++++++++++++++++++++++++++++------ 1 file changed, 52 insertions(+), 8 deletions(-) diff --git a/ocaml/xapi/context.ml b/ocaml/xapi/context.ml index 33fb5b0db26..120f877a5ed 100644 --- a/ocaml/xapi/context.ml +++ b/ocaml/xapi/context.ml @@ -187,6 +187,9 @@ let destroy __context = if not __context.forwarded_task then !__destroy_task ~__context __context.task_id +let hash_of_session_id session_id = + session_id |> Ref.string_of |> Digest.string |> Digest.to_hex + (* CP-982: create tracking id in log files to link username to actions *) let trackid_of_session ?(with_brackets = false) ?(prefix = "") session_id = match session_id with @@ -195,8 +198,7 @@ let trackid_of_session ?(with_brackets = false) ?(prefix = "") session_id = | Some session_id -> (* a hash is used instead of printing the sensitive session_id value *) let trackid = - Printf.sprintf "trackid=%s" - (Digest.to_hex (Digest.string (Ref.string_of session_id))) + Printf.sprintf "trackid=%s" (hash_of_session_id session_id) in if with_brackets then Printf.sprintf "%s(%s)" prefix trackid else trackid @@ -234,7 +236,36 @@ let parent_of_origin (origin : origin) span_name = | _ -> None -let start_tracing_helper parent_fn task_name = +let make_attributes ?task_name ?task_id ?task_uuid ?session_id ?origin () = + let attribute_helper_fn f v = Option.fold ~none:[] ~some:f v in + [ + attribute_helper_fn + (fun task_name -> [("xs.xapi.task.name", task_name)]) + task_name + ; attribute_helper_fn + (fun task_id -> [("xs.xapi.task.id", Ref.really_pretty_and_small task_id)]) + task_id + ; attribute_helper_fn + (fun task_uuid -> [("xs.xapi.task.uuid", Uuidx.to_string task_uuid)]) + task_uuid + ; attribute_helper_fn + (fun session_id -> + [("xs.xapi.session.track.id", hash_of_session_id session_id)] + ) + session_id + ; attribute_helper_fn + (fun origin -> + match origin with + | Internal -> + [("xs.xapi.task.origin", "internal")] + | Http _ -> + [("xs.xapi.task.origin", "http")] + ) + origin + ] + |> List.concat + +let start_tracing_helper ?(span_attributes = []) parent_fn task_name = let open Tracing in let span_details_from_task_name task_name = let uuid_length = 36 in @@ -242,9 +273,11 @@ let start_tracing_helper parent_fn task_name = let open String in if starts_with ~prefix:dispatch_system_is_alive task_name then let uuid = sub task_name (length dispatch_system_is_alive) uuid_length in - ("dispatch:system.isAlive", [("xs.span.arg.vm.uuid", uuid)]) + ( "dispatch:system.isAlive" + , ("xs.span.arg.vm.uuid", uuid) :: span_attributes + ) else - (task_name, []) + (task_name, span_attributes) in let span_name, span_attributes = span_details_from_task_name task_name in let parent = parent_fn span_name in @@ -276,7 +309,12 @@ let from_forwarded_task ?(http_other_config = []) ?session_id let dbg = make_dbg http_other_config task_name task_id in info "task %s forwarded%s" dbg (trackid_of_session ~with_brackets:true ~prefix:" " session_id) ; - let tracing = start_tracing_helper (parent_of_origin origin) task_name in + let span_attributes = + make_attributes ~task_id ~task_name ?session_id ~origin () + in + let tracing = + start_tracing_helper ~span_attributes (parent_of_origin origin) task_name + in { session_id ; task_id @@ -323,7 +361,12 @@ let make ?(http_other_config = []) ?(quiet = false) ?subtask_of ?session_id " by task " ^ make_dbg [] "" subtask_of ) ) ; - let tracing = start_tracing_helper (parent_of_origin origin) task_name in + let span_attributes = + make_attributes ~task_id ~task_name ~origin ?session_id ~task_uuid () + in + let tracing = + start_tracing_helper ~span_attributes (parent_of_origin origin) task_name + in { session_id ; database @@ -344,7 +387,8 @@ let make_subcontext ~__context ?task_in_database task_name = let tracing = Option.bind __context.tracing (fun parent -> let parent = Some parent in - start_tracing_helper (fun _ -> parent) task_name + let span_attributes = make_attributes ?session_id () in + start_tracing_helper ~span_attributes (fun _ -> parent) task_name ) in {subcontext with client= __context.client; tracing} From 0ea3ff8872caf382cf6bfbc16342029d15ed3c1c Mon Sep 17 00:00:00 2001 From: Gabriel Buica Date: Mon, 29 Jan 2024 15:12:09 +0000 Subject: [PATCH 2/2] CP-46631: Remove code duplication Removes code duplication by passing the exception error as an optional argument to `complete_tracing`. Improves code maitainability. Signed-off-by: Gabriel Buica --- ocaml/xapi/context.ml | 13 ++----------- ocaml/xapi/context.mli | 4 +--- ocaml/xapi/taskHelper.ml | 2 +- 3 files changed, 4 insertions(+), 15 deletions(-) diff --git a/ocaml/xapi/context.ml b/ocaml/xapi/context.ml index 120f877a5ed..4179cf7d930 100644 --- a/ocaml/xapi/context.ml +++ b/ocaml/xapi/context.ml @@ -50,17 +50,8 @@ type t = { ; mutable test_clusterd_rpc: (Rpc.call -> Rpc.response) option } -let complete_tracing __context = - ( match Tracing.Tracer.finish __context.tracing with - | Ok _ -> - () - | Error e -> - R.warn "Failed to complete tracing: %s" (Printexc.to_string e) - ) ; - __context.tracing <- None - -let complete_tracing_with_exn __context error = - ( match Tracing.Tracer.finish ~error __context.tracing with +let complete_tracing ?error __context = + ( match Tracing.Tracer.finish ?error __context.tracing with | Ok _ -> () | Error e -> diff --git a/ocaml/xapi/context.mli b/ocaml/xapi/context.mli index 3f2b2c5dcf9..7b2ece18c2c 100644 --- a/ocaml/xapi/context.mli +++ b/ocaml/xapi/context.mli @@ -142,9 +142,7 @@ val get_client_ip : t -> string option val get_user_agent : t -> string option -val complete_tracing : t -> unit - -val complete_tracing_with_exn : t -> exn * string -> unit +val complete_tracing : ?error:exn * string -> t -> unit val tracing_of : t -> Tracing.Span.t option diff --git a/ocaml/xapi/taskHelper.ml b/ocaml/xapi/taskHelper.ml index ee896269eec..abe7f4b4599 100644 --- a/ocaml/xapi/taskHelper.ml +++ b/ocaml/xapi/taskHelper.ml @@ -244,7 +244,7 @@ let cancel ~__context = let failed ~__context exn = let backtrace = Printexc.get_backtrace () in - Context.complete_tracing_with_exn __context (exn, backtrace) ; + Context.complete_tracing __context ~error:(exn, backtrace) ; let code, params = ExnHelper.error_of_exn exn in operate_on_db_task ~__context (fun self -> let status = Db_actions.DB_Action.Task.get_status ~__context ~self in