[CIVIS-2903] Add fiber-local context and public API to get active test #64

anmarchenko · 2023-11-03T13:32:33Z

What does this PR do?
Adds public Datadog::CI.active_test and Datadog::CI.active_span methods in order to provide an official way to get currently running span or test.

This will allow us to get rid of adhoc saving the current test in our instrumentations, for example like this:

# cucumber
@current_feature_span = Datadog::CI.start_test()
# ...
@current_feature_span.finish

or like this

# minitest
test_span = Datadog::CI.start_test()
Thread.current[:_datadog_test_span] = test_span
# ...
test_span = Thread.current[:_datadog_test_span]
test_span.finish

Now it should be done like this:

Datadog::CI.start_test()
# ...
test_span = Datadog::CI.active_test
test_span.finish

active_test is stored in CI-specific fiber-local context (see Datadog::CI::Context::Local)
active_span is fetched using existing Datadog::Tracing.active_span method

Motivation
Finishing the manual API for the current state of test-level visibility, start to handle CI-specific context in this library

How to test the change?
By running instrumented Ruby project and making sure nothing has changed in submitted tests

codecov-commenter · 2023-11-03T13:35:24Z

Codecov Report

Merging #64 (e1a9bb6) into main (ef6e234) will increase coverage by 0.03%.
The diff coverage is 98.53%.

@@            Coverage Diff             @@
##             main      #64      +/-   ##
==========================================
+ Coverage   99.16%   99.20%   +0.03%     
==========================================
  Files         106      110       +4     
  Lines        3606     3781     +175     
  Branches      131      140       +9     
==========================================
+ Hits         3576     3751     +175     
  Misses         30       30

Files	Coverage Δ
lib/datadog/ci.rb	`100.00% <100.00%> (ø)`
lib/datadog/ci/context/local.rb	`100.00% <100.00%> (ø)`
lib/datadog/ci/contrib/minitest/hooks.rb	`96.55% <100.00%> (-0.23%)`	⬇️
lib/datadog/ci/recorder.rb	`100.00% <100.00%> (ø)`
lib/datadog/ci/span.rb	`100.00% <100.00%> (ø)`
lib/datadog/ci/test.rb	`100.00% <100.00%> (ø)`
spec/datadog/ci/context/local_spec.rb	`100.00% <100.00%> (ø)`
...atadog/ci/contrib/cucumber/instrumentation_spec.rb	`100.00% <100.00%> (ø)`
...atadog/ci/contrib/minitest/instrumentation_spec.rb	`100.00% <100.00%> (ø)`
...c/datadog/ci/contrib/rspec/instrumentation_spec.rb	`100.00% <100.00%> (ø)`
... and 5 more

📣 Codecov offers a browser extension for seamless coverage viewing on GitHub. Try it in Chrome or Firefox today!

…ing with it

…:CI::Test object

… multiple examples

…ently active

ivoanjo · 2023-11-08T13:25:37Z

lib/datadog/ci/context/local.rb

+        def active_test
+          Thread.current[@key]
+        end
+
+        UNIQUE_INSTANCE_MUTEX = Mutex.new
+        UNIQUE_INSTANCE_GENERATOR = Datadog::Core::Utils::Sequence.new
+
+        private_constant :UNIQUE_INSTANCE_MUTEX, :UNIQUE_INSTANCE_GENERATOR
+
+        def self.next_instance_id
+          UNIQUE_INSTANCE_MUTEX.synchronize { UNIQUE_INSTANCE_GENERATOR.next }
+        end
+
+        private
+
+        def active_test=(test)
+          Thread.current[@key] = test
+        end


I wonder if we could get away with not having a unique id per context. Is it really that common that we'll have more than one context alive in an app?

I'm asking because this has the same potential memory leak issues I shared in our private chat; namely, that the unique keys will live forever until the Ruby process dies.

it is very convenient for testing though, not sure how often would it be used in real world case

what is important for us is that CI session is short lived so there would be one recorder instance per forked process and there is no real danger in creating really bad memory leak in my opinion

Note that I'm not saying to create a single context forever; what I'm saying is, if the semantics is that in the usual case only one context would be alive, it seems like you could reuse the key?

yes, this is true, let me try to do that and see how many tests break...

@ivoanjo it worked without issues and made the code simpler! thank you for the suggestion

Cool, glad to know! The profiler does something similar (for ~slightly different reasons) and it worked out reasonably as well.

…hat it does

romainkomorn-exdatadog · 2023-11-10T08:47:03Z

lib/datadog/ci/recorder.rb

-      private
-
-      def create_datadog_span(span_name, span_options: {}, tags: {}, &block)
+        # create_datadog_span(span_name, span_options: span_options, tags: tags, &block)


Is this meant as documentation or just left behind?

thank you! this should not be here

marcotc · 2023-11-10T17:21:53Z

README.md

-  # Enables the RSpec instrumentation
-  c.ci.instrument :rspec, **options
+# Only activates test instrumentation on CI
+if ENV["DD_ENV"] == "ci"


Offtopic: what do you think of making this check ENV["DD_ENV"] == "ci" by default internally, and assume most clients want this? And then provide an option to override it, for the exceptional clients that don't want this.

I don't think making internal assumptions on when to turn things on or off based on DD_ENV would make sense to me as a user. It's much more likely I'd end up wondering why things aren't working because I missed the part in the docs that says DD_ENV has to be set and equal to ci for things to work.

We don't make a lot of (if any) assumptions/decisions on the value of DD_ENV in our tracers' CI Visibility integrations.

agree with @romainkomorndatadog, there is a very fine line between doing "magic" and surprising customers with unexpected default behaviour. Many clients use "test" as a DD_ENV value for CI for example. Here in the docs we just want to point out that they should enable CI visibility mode only when running on CI to avoid local test runs to be reported.

anmarchenko force-pushed the anmarchenko/context_provider branch from 3525a0d to 9cc22eb Compare November 6, 2023 08:30

copy context_provider and context from ddtrace-rb to start experiment…

a1a2857

…ing with it

anmarchenko force-pushed the anmarchenko/context_provider branch from 9cc22eb to a1a2857 Compare November 7, 2023 08:09

implement local context to keep track of current test being executed

4e70b4b

anmarchenko changed the title ~~WIP context~~ Add fiber-local context and public API to get active test Nov 7, 2023

anmarchenko added 8 commits November 7, 2023 16:00

Recorder#build_test is a factory method that creates correct Datadog:…

7ceca4a

…:CI::Test object

factory method for Datadog::CI::Span

8059258

Span#to_s method

fea20a4

add expectations that environment tags are not collected when running…

f021d49

… multiple examples

public api documentation

a91b81f

remove mentions of automatic trace metrics from README

a18acfe

remove confusing c.tracing.enabled setting from README

c60e824

add comment that tracing a test returns error if another test is curr…

d113122

…ently active

anmarchenko marked this pull request as ready for review November 8, 2023 09:20

anmarchenko requested review from a team and juan-fernandez November 8, 2023 09:20

ivoanjo reviewed Nov 8, 2023

View reviewed changes

anmarchenko changed the title ~~Add fiber-local context and public API to get active test~~ [CIVIS-2903] Add fiber-local context and public API to get active test Nov 8, 2023

anmarchenko added 4 commits November 8, 2023 17:12

use a single local context

a64abeb

cleanup redundant require

b61b2c7

rename set_internal_tracing_context! to set_trace_origin as this is w…

8b5636e

…hat it does

freeze the environment tags hash

8a8ab5c

romainkomorn-exdatadog approved these changes Nov 10, 2023

View reviewed changes

remove unncessary comment

e1a9bb6

marcotc reviewed Nov 10, 2023

View reviewed changes

anmarchenko merged commit 7bcfb29 into main Nov 20, 2023
11 checks passed

anmarchenko deleted the anmarchenko/context_provider branch November 20, 2023 13:09

github-actions bot added this to the 0.4.0 milestone Nov 20, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CIVIS-2903] Add fiber-local context and public API to get active test #64

[CIVIS-2903] Add fiber-local context and public API to get active test #64

anmarchenko commented Nov 3, 2023 •

edited

Loading

codecov-commenter commented Nov 3, 2023 •

edited

Loading

ivoanjo Nov 8, 2023

anmarchenko Nov 8, 2023

ivoanjo Nov 8, 2023

anmarchenko Nov 8, 2023

anmarchenko Nov 8, 2023

ivoanjo Nov 8, 2023

romainkomorn-exdatadog Nov 10, 2023

anmarchenko Nov 10, 2023

marcotc Nov 10, 2023 •

edited

Loading

romainkomorn-exdatadog Nov 13, 2023

romainkomorn-exdatadog Nov 13, 2023

anmarchenko Nov 20, 2023

[CIVIS-2903] Add fiber-local context and public API to get active test #64

[CIVIS-2903] Add fiber-local context and public API to get active test #64

Conversation

anmarchenko commented Nov 3, 2023 • edited Loading

codecov-commenter commented Nov 3, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcotc Nov 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anmarchenko commented Nov 3, 2023 •

edited

Loading

codecov-commenter commented Nov 3, 2023 •

edited

Loading

marcotc Nov 10, 2023 •

edited

Loading