[SDTEST-160] git commands telemetry #205
Conversation
…alization causes deadlock. Made environment tags collection lazy, now it happens the first time it needed for any CI span.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## anmarchenko/telemetry_metrics_itr #205 +/- ##
=====================================================================
- Coverage 98.86% 98.83% -0.04%
=====================================================================
Files 249 251 +2
Lines 11279 11416 +137
Branches 503 509 +6
=====================================================================
+ Hits 11151 11283 +132
- Misses 128 133 +5 ☔ View full report in Codecov by Sentry. |
| res = nil | ||
|
|
||
| duration_ms = Core::Utils::Time.measure(:float_millisecond) do | ||
| res = exec_git_command("git ls-remote --get-url") |
There was a problem hiding this comment.
Small question: why not have exec_git_command() do the measuring and return some kind of tuple with result, duration, and status code? Or alternatively, you could pass the git command as a parameter and have the underlying method do the telemetry piece.
It would be a very modest win in code deduplication.
There was a problem hiding this comment.
I'm not a fan of DRY in general and I eager to accept code duplication every time over confusing behaviour.
Changing return of the method to include telemetry mixes 2 different concepts together: executing command and measuring it. If I return back to this code after 2 months, I will never remember why exec_git_command returns a tuple. What's worse, we measure only subset of git commands (for whatever reason). This means that these return values will be used in some cases but ignored in other and it will be absolutely impossible to explain why.
In this case deduplicating the code would completely break exec_git_command abstraction: I will take duplication over broken abstraction every day (like Sandi taught me! :) )
There was a problem hiding this comment.
I ended up inserting a wrapped function for the Python tracer... but since I had to maintain backwards compatibility, I kept the original function as a wrapper of the new one, which may or may not have been the right move.
There was a problem hiding this comment.
I'm fine with duplication for now, but I will refactor if it'll become painful in the future
What does this PR do?
Adds telemetry metrics collection to git commands
Additional Notes
This change caused curious fail: one must not access Telemetry when the library is initialized. We previously collected environment tags during the library initialization stage, which might include using git commands. I made the environment tags collection lazy, so it does not happen until at least one span is traced.
How to test the change?
Unit tests are provided