Skip to content
This repository has been archived by the owner on Aug 30, 2022. It is now read-only.

refactor tracing #492

Merged
merged 1 commit into from
Aug 10, 2020
Merged

refactor tracing #492

merged 1 commit into from
Aug 10, 2020

Conversation

little-dude
Copy link
Contributor

use of the Traced struct has two downsides:

  • Because we want to decouple the tracing logic from the application
    logic, we introduced generics all over the place for our code to
    work both with T and Traced.
  • We currently pass the Traced.span() down all the services chain, but
    Spans are more of less immutable and declared in macros at compile
    time, so just passing this span down is useless: we cannot at any
    field to it.

This refactoring is twofold:

  1. Remove all the genericity around tracing. This resulted in a lot of
    simplifications, especially in the state machine, but also in the
    pet message services since we could get rid of the
    _PetMessageHandler trait.

  2. Replace Traced by a Request that is going to be used
    everywhere. Note that we currently just use Request in for the
    PET message services and the state machine, but it should be
    straightforward to use anywhere else. Unlike Traced<T>,
    Request<T> allows us to enrich the span attached to the request:
    Request::map creates a child span a the request is mapped.

@little-dude little-dude marked this pull request as ready for review August 6, 2020 14:15
@codecov
Copy link

codecov bot commented Aug 6, 2020

Codecov Report

Merging #492 into master will increase coverage by 1.99%.
The diff coverage is 52.68%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #492      +/-   ##
==========================================
+ Coverage   51.56%   53.55%   +1.99%     
==========================================
  Files          63       63              
  Lines        3105     3049      -56     
==========================================
+ Hits         1601     1633      +32     
+ Misses       1504     1416      -88     
Impacted Files Coverage Δ
rust/src/client/request.rs 9.63% <0.00%> (ø)
rust/src/message/message.rs 39.28% <0.00%> (-6.55%) ⬇️
rust/src/rest.rs 0.00% <ø> (ø)
rust/src/services/messages/mod.rs 0.00% <0.00%> (ø)
rust/src/services/messages/state_machine.rs 0.00% <0.00%> (ø)
rust/src/services/mod.rs 0.00% <ø> (ø)
rust/src/state_machine/mod.rs 45.00% <0.00%> (ø)
rust/src/utils/request.rs 54.54% <54.54%> (ø)
rust/src/state_machine/phases/idle.rs 90.90% <60.00%> (ø)
rust/src/state_machine/phases/error.rs 85.71% <66.66%> (ø)
... and 11 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ab20f52...ee7a275. Read the comment docs.

Copy link
Contributor

@Robert-Steiner Robert-Steiner left a comment

Choose a reason for hiding this comment

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

As already mentioned, really nice improvement!
I only added two questions

rust/src/utils/request.rs Show resolved Hide resolved
rust/src/services/messages/message_parser.rs Show resolved Hide resolved
Copy link
Contributor

@finiteprods finiteprods left a comment

Choose a reason for hiding this comment

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

great - I like that we can remove a lot of code by being less generic! especially in the state machine.
just spotted some undocumented types along the way, nothing major.

rust/src/services/messages/message_parser.rs Show resolved Hide resolved
rust/src/services/messages/message_parser.rs Outdated Show resolved Hide resolved
rust/src/services/messages/state_machine.rs Show resolved Hide resolved
rust/src/state_machine/phases/idle.rs Outdated Show resolved Hide resolved
rust/src/state_machine/requests.rs Outdated Show resolved Hide resolved
rust/src/utils/request.rs Show resolved Hide resolved
rust/src/utils/request.rs Show resolved Hide resolved
use of the `Traced` struct has two downsides:

- Because we want to decouple the tracing logic from the application
  logic, we introduced generics all over the place for our code to
  work both with T and Traced<T>.
- We currently pass the `Traced.span()` down all the services chain, but
  `Span`s are more of less immutable and declared in macros at compile
  time, so just passing this span down is useless: we cannot at any
  field to it.

This refactoring is  twofold:

1. Remove all the genericity around tracing. This resulted in a lot of
   simplifications, especially in the state machine, but also in the
   pet message services since we could get rid of the
   `_PetMessageHandler` trait.

2. Replace Traced<T> by a Request<T> that is going to be used
   everywhere. Note that we currently just use `Request` in for the
   PET message services and the state machine, but it should be
   straightforward to use anywhere else. Unlike `Traced<T>`,
   `Request<T>` allows us to enrich the span attached to the request:
   `Request::map` creates a child span a the request is mapped.
@little-dude little-dude merged commit 8292650 into master Aug 10, 2020
@little-dude little-dude deleted the refactor-tracing branch August 10, 2020 12:13
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants