CP-48195: Instrument client side of forkexecd
#5583
Conversation
GabrielBuica
force-pushed
the
private/dbuica/CP-48195-instrument-forkexecd-client
branch
from
45eeca7
to
0f771ab
Compare
|
|
|
Passed BVT+BST 197518. |
GabrielBuica
force-pushed
the
private/dbuica/CP-48195-instrument-forkexecd-client
branch
from
b9defa1
to
e800417
Compare
ocaml/libs/tracing/tracing.ml
Outdated
| @@ -573,7 +575,7 @@ let set ?enabled ?attributes ?endpoints ~uuid () = | |||
| Hashtbl.clear Spans.spans ; | |||
| Hashtbl.clear Spans.finished_spans | |||
| ) | |||
| ) else | |||
| ) else if not (get_observe ()) then | |||
There was a problem hiding this comment.
It is fine to always call set_observe here. get + set is not atomic as a whole anyway (if you want it to be atomic, then there is compare_and_set).
But this flag is only written from one location and read from multiple locations, and it is not a computation that can race between different threads, so always calling set is probably better.
ocaml/xapi/xapi_observer.ml
Outdated
| @@ -309,6 +309,7 @@ module Dom0ObserverConfig (ObserverComponent : OBSERVER_COMPONENT) : | |||
|
|
|||
| let update_config ~__context ~observer ~uuid = | |||
| if Db.Observer.get_enabled ~__context ~self:observer then ( | |||
| if not (Tracing.get_observe ()) then Tracing.set_observe true ; | |||
There was a problem hiding this comment.
get + set is not atomic, it is better to just call set directly, so you know what the end result is.
This isn't performance critical code in a tight loop where optimizing that would make a difference (even there it is questionable whether it would)
There was a problem hiding this comment.
The equivalent atomic operation in this case would be let _ = Atomic_compare_and_set observe false true in
ocaml/libs/tracing/tracing.ml
Outdated
| @@ -573,7 +575,7 @@ let set ?enabled ?attributes ?endpoints ~uuid () = | |||
| Hashtbl.clear Spans.spans ; | |||
| Hashtbl.clear Spans.finished_spans | |||
| ) | |||
| ) else | |||
| ) else if not (get_observe ()) then | |||
| set_observe true | |||
There was a problem hiding this comment.
The set call here can race with another set call from the API. It is better to do the (atomic) set operation while you are holding the lock. Otherwise if one operation would enable things and the other would disable you'd still have a race.
The atomic only protects the global value itself, not the way it was computed, adding atomic doesn't make the operation itself atomic (that is the responsibility of the code).
There was a problem hiding this comment.
In fact 'get_tracer_providers' above is not thread-safe: it uses a hashtable, and iterates on it without holding any locks.
GabrielBuica
force-pushed
the
private/dbuica/CP-48195-instrument-forkexecd-client
branch
2 times, most recently
from
1c14e7c
to
aa22166
Compare
|
This PR could be rebased on top of this one that first fixes the locking bugs in tracing.ml, so then the changes in this PR have a better starting point: #5601 |
GabrielBuica
force-pushed
the
private/dbuica/CP-48195-instrument-forkexecd-client
branch
from
aa22166
to
4a6c898
Compare
Since the number of changings to |
Instrument: - `forkhelpers.ml`, - `fecomms.ml` to create spans around functions when a parent span is supplied as `tracing`. Signed-off-by: Gabriel Buica <danutgabriel.buica@cloud.com>
GabrielBuica
force-pushed
the
private/dbuica/CP-48195-instrument-forkexecd-client
branch
from
4a6c898
to
9ac7c64
Compare
Currently the `observe` mode of the `tracing` library do not work corrently resulting in the logs being spammed by this warning. Comment it out so that the logs do not become too big (for the time being). Signed-off-by: Gabriel Buica <danutgabriel.buica@cloud.com>
GabrielBuica
force-pushed
the
private/dbuica/CP-48195-instrument-forkexecd-client
branch
from
9ac7c64
to
70f8149
Compare
Instrument:
forkhelpers.ml,fecomms.mlto create spans around functions when a parent span is supplied as
tracing.As follow-up to #5551