editoast: hide 'datadog' and 'opentelemetry' behind feature flags

[woshilapin]

The goal is to reduce the default compile time for developers.
Datadog is mostly used in specific deployment, not necessary to run any
test or any useful runtime functionalities unless communicating with
a Datadog agent.

Some summary about the total number of dependencies depending on which
feature is activated:

activated features	number of dependencies
(None)	566
datadog	582
opentelemetry	596
datadog, opentelemetry	604

[codecov-commenter]

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.49%. Comparing base (ce6ae71) to head (a8ff592).
Report is 5 commits behind head on dev.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@           Coverage Diff           @@
##              dev    #8598   +/-   ##
=======================================
  Coverage   87.49%   87.49%           
=======================================
  Files          31       31           
  Lines        1535     1535           
=======================================
  Hits         1343     1343           
  Misses        192      192           

Flag	Coverage Δ
railjson_generator	87.49% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[leovalais]

LGTM (editoast, not tested)

[flomonster]

Do you have stats on the compile time with/without these dependencies? I feel it won't change much since most of the compile time is spent on osrd (not the dependencies).

We might add complexity without real gain. From my point of view, the pain comes from the rustanalyzer runtime rather than the build time.

[woshilapin]

Do you have stats on the compile time with/without these dependencies? I feel it won't change much since most of the compile time is spent on osrd (not the dependencies).

We might add complexity without real gain. From my point of view, the pain comes from the rustanalyzer runtime rather than the build time.

I agree that for rust-analyzer it might not be a big impact. With these 2 benchmarks, it shows no effect (if you check the entire workspace or not).

❯ hyperfine --warmup 1 --parameter-list branch dev,wsl/editoast/build/no-default-datadog --setup 'git switch {branch}' --prepare "fd '.*\.rs' --exec touch '{}' ';'" 'cargo check --workspace'

Benchmark 1: cargo check --workspace (branch = dev)
  Time (mean ± σ):     22.991 s ±  3.734 s    [User: 23.251 s, System: 1.138 s]
  Range (min … max):   18.889 s … 31.894 s    10 runs

Benchmark 2: cargo check --workspace (branch = wsl/editoast/build/no-default-datadog)
  Time (mean ± σ):     23.035 s ±  3.541 s    [User: 23.241 s, System: 1.091 s]
  Range (min … max):   18.356 s … 28.058 s    10 runs

Summary
  cargo check --workspace (branch = dev) ran
    1.00 ± 0.22 times faster than cargo check --workspace (branch = wsl/editoast/build/no-default-datadog)

❯ hyperfine --warmup 1 --parameter-list branch dev,wsl/editoast/build/no-default-datadog --setup 'git switch {branch}' --prepare "fd '.*\.rs' --exec touch '{}' ';'" 'cargo check'

Benchmark 1: cargo check (branch = dev)
  Time (mean ± σ):     22.497 s ±  2.661 s    [User: 22.300 s, System: 1.032 s]
  Range (min … max):   18.330 s … 26.341 s    10 runs

Benchmark 2: cargo check (branch = wsl/editoast/build/no-default-datadog)
  Time (mean ± σ):     21.864 s ±  2.106 s    [User: 21.662 s, System: 1.033 s]
  Range (min … max):   17.721 s … 24.563 s    10 runs

Summary
  cargo check (branch = wsl/editoast/build/no-default-datadog) ran
    1.03 ± 0.16 times faster than cargo check (branch = dev)

But rust-analyzer is not the only case where this might matters. For example, we often run (and therefore compile) the tests... which means there is linking happening. Therefore, less dependencies might be important in this case. Here are the numbers (notice the --no-run in order to really measure the compile time).

 ❯ hyperfine --warmup 1 --parameter-list branch dev,wsl/editoast/build/no-default-datadog --setup 'git switch {branch}' --prepare "fd '.*\.rs' --exec touch '{}' ';'" 'cargo test --no-run'

Benchmark 1: cargo test --no-run (branch = dev)
  Time (mean ± σ):     46.629 s ±  2.917 s    [User: 54.822 s, System: 2.596 s]
  Range (min … max):   42.925 s … 50.927 s    10 runs

Benchmark 2: cargo test --no-run (branch = wsl/editoast/build/no-default-datadog)
  Time (mean ± σ):     44.674 s ±  4.304 s    [User: 52.171 s, System: 2.437 s]
  Range (min … max):   36.253 s … 53.162 s    10 runs

Summary
  cargo test --no-run (branch = wsl/editoast/build/no-default-datadog) ran
    1.04 ± 0.12 times faster than cargo test --no-run (branch = dev)

We see there is no significant improvement (the difference in time is smaller than the variance).

⚠️ UPDATED [31/08/2024]: all benchmarks were wrongly executed on an outdated dev branch. The numbers above have been updated with a better benchmark.

[leovalais]

My speedup factor is less surprising, but 3% just for the telemetry still seems quite a lot tbh.

❯ hyperfine --warmup 1 --parameter-list branch 'dev,wsl/editoast/build/no-default-datadog' --setup 'git switch {branch}' --prepare "fd '.*\.rs' --exec touch '{}' ';'" 'cargo test --no-run'

Benchmark 1: cargo test --no-run (branch = dev)
  Time (mean ± σ):     23.636 s ±  1.564 s    [User: 42.207 s, System: 6.771 s]
  Range (min … max):   19.469 s … 25.034 s    10 runs

Benchmark 2: cargo test --no-run (branch = wsl/editoast/build/no-default-datadog)
  Time (mean ± σ):     23.000 s ±  0.493 s    [User: 39.789 s, System: 6.363 s]
  Range (min … max):   22.442 s … 24.086 s    10 runs

Summary
  cargo test --no-run (branch = wsl/editoast/build/no-default-datadog) ran
    1.03 ± 0.07 times faster than cargo test --no-run (branch = dev)

[woshilapin]

I updated comment since the result were plainly wrong. In the end, there is no situation in which removing some dependencies has an impact on the performance. Therefore, the comment of @flomonster is completely relevant.

Even the results obtained by @leovalais don't show a significant result, the variance is greater than the time difference. Just to be sure, I increased the number of calculation in a benchmark and even remove compilation of sub-crates from the equation (just touch file in editoast/src instead of all the rust files in sub-crates). See the results, it's a plain 0% improvement.

❯ hyperfine --min-runs 42 --warmup 1 --parameter-list branch dev,wsl/editoast/build/no-default-datadog --setup 'git switch {branch}' --prepare "fd '.*\.rs' src/ --exec touch '{}' ';'" 'cargo test --no-run'

Benchmark 1: cargo test --no-run (branch = dev)
  Time (mean ± σ):     44.930 s ±  4.062 s    [User: 52.078 s, System: 1.966 s]
  Range (min … max):   33.290 s … 53.586 s    42 runs

Benchmark 2: cargo test --no-run (branch = wsl/editoast/build/no-default-datadog)
  Time (mean ± σ):     44.949 s ±  4.553 s    [User: 51.683 s, System: 1.960 s]
  Range (min … max):   32.626 s … 58.047 s    42 runs

Summary
  cargo test --no-run (branch = dev) ran
    1.00 ± 0.14 times faster than cargo test --no-run (branch = wsl/editoast/build/no-default-datadog)

Now, I'm ok to drop the opentelemetry feature as it brings significant complexity for no improvement. However, I'd be ok to discuss keeping the datadog feature for the following 2 reasons:

• the commit to make Datadog optional is much more simple than for OpenTelemetry (see for yourself 7760192).
• Datadog being some non-standard proprietary solution, only needed in some specific deployment situations, I'd say it reasonable to deactivate it by default in an open-source project

I'm ok either way. Note that this PR still also contains 2 small fix commits anyway so even if we remove the entire featurization of Datadog and OpenTelemetry, we might want to merge it still (with an edit of the title maybe 🤷).

[leovalais]

Too bad it doesn't lead to any improvement. Agreed about the datadog feature.

[flomonster]

Same as @leovalais. Another reason to keep a feature flag for datadog is to make it easier to remove datadog soon (at least I hope so).