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

Ensure public apis don't fail after shutdown #1408

Merged
merged 4 commits into from
Mar 17, 2021
Merged

Conversation

marcotc
Copy link
Member

@marcotc marcotc commented Mar 15, 2021

Our public API can be use permissively across the host application. During the event of a Ruby process shutdown, ddtrace will invoke Datadog.shutdown! (though an at_exit hook) to gracefully flush any pending traces and decommission resources.

When this shutdown happens, internal parts of the tracer won't work anymore (new traces won't be captured), which is acceptable, but we must still ensure that calls out public API methods do not raise errors and still behave predictably during a shutdown. We don't want the tracer to interfere with any of the host application's own shutdown routines by introducing uncertain behavior in our tracer public methods.

This PR ensures that our publicly exposed API does not change their interface, nor raise errors after the tracer has been shutdown.

@marcotc marcotc added the dev/testing Involves testing processes (e.g. RSpec) label Mar 15, 2021
@marcotc marcotc requested a review from a team March 15, 2021 23:05
@marcotc marcotc self-assigned this Mar 15, 2021
ivoanjo
ivoanjo previously approved these changes Mar 16, 2021
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.

I must admit the current behavior is a bit... weird, but as far as this test goes, it LGTM 👍

end

it 'does not error on logging' do
expect(Datadog.logger.info('test')).to be_truthy
Copy link
Member

Choose a reason for hiding this comment

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

@ivoanjo
Copy link
Member

ivoanjo commented Mar 16, 2021

Looking at the CI failures, at least the JRuby failure seems to be legit.

@codecov-io
Copy link

Codecov Report

Merging #1408 (c7d1bba) into master (d015782) will decrease coverage by 0.00%.
The diff coverage is 97.87%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1408      +/-   ##
==========================================
- Coverage   98.16%   98.16%   -0.01%     
==========================================
  Files         768      769       +1     
  Lines       36667    36714      +47     
==========================================
+ Hits        35993    36039      +46     
- Misses        674      675       +1     
Impacted Files Coverage Δ
spec/ddtrace_integration_spec.rb 97.87% <97.87%> (ø)

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 d015782...c7d1bba. Read the comment docs.

ivoanjo
ivoanjo previously approved these changes Mar 17, 2021
@marcotc marcotc merged commit 768cf9b into master Mar 17, 2021
@marcotc marcotc deleted the fatal-stays-fatal branch March 17, 2021 16:49
@github-actions github-actions bot added this to the 0.47.0 milestone Mar 17, 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