Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[test] Wait for AsyncTraceWriter to finish sending traces #1422

Merged
merged 2 commits into from
Mar 22, 2021

Conversation

marcotc
Copy link
Member

@marcotc marcotc commented Mar 19, 2021

This PR addresses an old, but recently more pronounced flaky test, regarding a Datadog::Workers::AsyncTraceWriter test.

This this was trying to wait until the writer had flushed all data, but instead, was waiting until the writer had started processing all traces (by calling #work_pending?).

#work_pending? is used internally by the writer to know if there are traces queued to be process, thus allowing the tracer to either go on a busy processing loop, or sleep for a short bit. This behaviour describes correctly if "Is there more work to be done next?", which is exactly what it represents, but does not answer "Is all the work done?".

This PR waits for the tracer to completely finish its work before verifying the desired conditions.

There's no impact on the correctness of the AsyncTraceWriter, as it is behaving as expected.

@marcotc marcotc added the dev/testing Involves testing processes (e.g. RSpec) label Mar 19, 2021
@marcotc marcotc self-assigned this Mar 19, 2021
@marcotc marcotc requested a review from a team March 19, 2021 19:16
@codecov-io
Copy link

Codecov Report

Merging #1422 (1a29c0b) into master (c0762e7) will decrease coverage by 0.00%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1422      +/-   ##
==========================================
- Coverage   98.13%   98.13%   -0.01%     
==========================================
  Files         771      771              
  Lines       36929    36930       +1     
==========================================
  Hits        36242    36242              
- Misses        687      688       +1     
Impacted Files Coverage Δ
spec/ddtrace/workers/trace_writer_spec.rb 95.18% <0.00%> (-0.28%) ⬇️
lib/ddtrace/patcher.rb 96.66% <0.00%> (-3.34%) ⬇️
...b/action_pack/action_controller/instrumentation.rb 95.71% <0.00%> (+1.42%) ⬆️

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 c0762e7...1a29c0b. Read the comment docs.

Copy link
Member

@ivoanjo ivoanjo left a comment

Choose a reason for hiding this comment

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

Big thanks for fixing this. I had seen it, but ended up not looking into it, but was really getting annoyed. Much ❤️ .

Comment on lines +29 to 32
# Are there more items to be processed next?
def work_pending?
!buffer.empty?
end
Copy link
Member

Choose a reason for hiding this comment

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

I'm a big fan of self-describing code. If we like the comment, why not turning this into def more_items_to_be_proccessed? or similar? :)

Copy link
Member Author

Choose a reason for hiding this comment

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

I do agree that comments <<< well-named-things.

I mean, isn't "more_items_to_be_processed?" =~ "work_pending?" 😄 I think the main issue is that, like all things complex, there's nuance that would be hard to capture in the name.

Maybe "more_items_to_be_processed?" or similar would be better, but I'm sneaking away with the ✅ for now for the non-flaky CI.

@marcotc marcotc merged commit e99e544 into master Mar 22, 2021
@marcotc marcotc deleted the fix-flake-async branch March 22, 2021 17:42
@github-actions github-actions bot added this to the 0.47.0 milestone Mar 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dev/testing Involves testing processes (e.g. RSpec)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants