Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Instrument task related functionality #5735

Merged

Conversation

GabrielBuica
Copy link
Contributor

Instruments task related functions.

Some other improvements:

  • Context.with_tracing now records the error inside the span;
  • Context.complete_tracing is now called with finally.

ocaml/xapi/taskHelper.ml Outdated Show resolved Hide resolved
ocaml/xapi/taskHelper.ml Outdated Show resolved Hide resolved
@contificate
Copy link
Contributor

Pedantically, I'd prefer we worked towards avoiding infix usage of @@ as it's an eyesore.

We can use let (let*) = (@@) and write let* __context = e in e'. Would be interested to hear what others think (even if the topic of syntax is as bikeshedding as it gets).

@lindig
Copy link
Contributor

lindig commented Jun 26, 2024

I don't have strong opinions on @@; I think the usage here was appropriate to wrap existing code. With the suggestion, we might run into the problem that let* is used for other purposes but we would use let@, I assume.

@GabrielBuica
Copy link
Contributor Author

I quite like the visual of @@ for with_tracing functions as they are mostly called at the start of the functions and you know that the entire scope will be inside at span. The downside is that it's not easy to follow that the argument is being changed: span/__context/dbg/etc.

As far as I am aware let* is used as a bind operator and from a quick search through the codebase let@ is defined as let ( let@ ) f x = f x . So, if we agree that a custom let operator is more readable I'd use the later.

ocaml/xapi/taskHelper.ml Outdated Show resolved Hide resolved
@GabrielBuica GabrielBuica force-pushed the private/dbuica/CA-394169-tracing branch 3 times, most recently from e9b44fa to c3c5346 Compare July 1, 2024 10:26
ocaml/xapi/taskHelper.ml Outdated Show resolved Hide resolved
ocaml/xapi/message_forwarding.ml Show resolved Hide resolved
@Vincent-lau
Copy link
Contributor

Looks good to me, but might need another eye from some one who's familiar with tracing as it is a relatively large change

Copy link
Contributor

@edwintorok edwintorok left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, although we should test that exceptions from the tracing module don't cause failures.

ocaml/xapi/taskHelper.ml Outdated Show resolved Hide resolved
ocaml/xapi/context.ml Outdated Show resolved Hide resolved
ocaml/xapi/message_forwarding.ml Show resolved Hide resolved
ocaml/xapi/taskHelper.ml Outdated Show resolved Hide resolved
Copy link
Contributor

@edwintorok edwintorok left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, can you squash the fixups into their respective commits (a git rebase should do that).

Use `finally` to execute `complete_tracing` on the context when we are
done and not at the beginning.

This allows for instrumentation and correct trace display of functions
such as `assert_op_valid`.

Signed-off-by: Gabriel Buica <danutgabriel.buica@cloud.com>
Update `with_tracing` to record the error and finish the span if the
instrumented function fails.

Add `session.track.id` and `task.id` attributes to all spans inside a
trace that is following a task.

This will result in spans being finished even if the function has raised
an exception. Plus, better debuggability in the case that something
fails.

Signed-off-by: Gabriel Buica <danutgabriel.buica@cloud.com>
Instruments task relatated functions.

This will allow for better debuggablity and understanding of issues related
with tasks.

Signed-off-by: Gabriel Buica <danutgabriel.buica@cloud.com>
`Date.now` is preffered for synchronisation between host as it uses
`ptime`.

Signed-off-by: Gabriel Buica <danutgabriel.buica@cloud.com>
Uses `let@` and `match` statements to avoid nesting.

This should increase code readability.

Signed-off-by: Gabriel Buica <danutgabriel.buica@cloud.com>
@GabrielBuica GabrielBuica force-pushed the private/dbuica/CA-394169-tracing branch from 7e819a2 to d496db1 Compare July 15, 2024 08:38
@robhoes robhoes merged commit 0c65928 into xapi-project:master Jul 16, 2024
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants