do not collect environment tags when CI is not enabled #87
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #87 +/- ##
=======================================
Coverage 99.13% 99.13%
=======================================
Files 135 135
Lines 5220 5226 +6
Branches 188 189 +1
=======================================
+ Hits 5175 5181 +6
Misses 45 45 ☔ View full report in Codecov by Sentry. |
| @enabled = enabled | ||
| @test_suite_level_visibility_enabled = enabled && test_suite_level_visibility_enabled | ||
|
|
||
| @environment_tags = Ext::Environment.tags(ENV).freeze | ||
| @environment_tags = @enabled ? Ext::Environment.tags(ENV).freeze : {} | ||
| @local_context = Context::Local.new | ||
| @global_context = Context::Global.new | ||
| end |
There was a problem hiding this comment.
I haven't looked at the CI stuff in a while, so pardon me if I'm asking a stupid question but... I don't understand -- why is a TestVisibility::Recorder being instanced at all for folks which are not running CI?
E.g. have you considered doing something like what we're doing for profiling
...where we don't instance anything if it's off?
I'm asking because this seems to have a potential to be a sharp edge. E.g. if we're skipping things one-by-one when CI is disabled, it seems easy to "oops" other things that shouldn't be loaded as well, and it seems a bit of a potential whack-a-mole of needing to remember "hey this gets instanced even when CI is not in use, so remember to disable the things that need to be disabled!".
There was a problem hiding this comment.
Yes, I think this was not the right call.
Let us ship this fix now and I made a note for myself to remove Recorder instance completely when CI is not enabled.
My "genious" thinking was that Datadog::CI calls will fail when there is no recorder instance when running tests with CI instrumentation disabled. This is an expected use case: we enable CI instrumentation only in CI environments and we run tests locally without instrumentation. So this means that when users use Datadog::CI entrypoint to manually instrument their tests we need to make sure that these calls do not fail. As I hate nil checks everywhere, I decided to just let Recorder be there, just skip traces creation when CI is not enabled.
Of course, I did not remember the fact that Recorder's constructor has a side effect: it collects environment tags and validates them. It is done in the constructor for performance reasons to make sure that they are being collected in the beginning of test run and cached for the whole run with no risk of this call being made multiple times.
I guess better way to make Datadog::CI calls work when CI instrumentation is disabled would be to introduce ni-checks or introduce a NullRecorder class that would return NullSpan all the time to skip tracing. Anyway, this is a topic for a follow-up PR and I will definitely do it before 0.6.0 version of datadog-ci gem.
anmarchenko
deleted the
anmarchenko/do_not_validate_environment_when_ci_disabled
branch
Fixes DataDog/dd-trace-rb#3314
What does this PR do?
Prevents Datadog::CI::Ext::Environment from collecting environment information and validating it if CI mode is not enabled.
Motivation
Regression introduced in 0.5.0 release: ddtrace started to emit error logs in production about CI environment tags not being provided
How to test the change?
Run ruby application with ddtrace in production mode