Ensure traces sent before process ends #195

ryplo · 2017-09-22T21:06:27Z

Overview

Fix for a problem found when implementing the Resque integration - the Resque process would end before the thread could finish sending the traces. This PR is to implement a shutdown method similar to that of the Python client: https://github.com/DataDog/dd-trace-py/blob/master/ddtrace/writer.py#L104-L121

Major changes

To ensure trace sending is completed before a process exits, call the shutdown method after the span should be finished.

Datadog.tracer.trace('web.request', service: 'my-web-site') do |span|
  do_something
end

Datadog.tracer.shutdown!

You can pass it in a tracer or the method will just use the default Datadog.tracer:
def self.shutdown(tracer = Datadog.tracer)

User impact

Trace sending completion is ensured for short-running processes

palazzem · 2017-09-25T19:27:50Z

lib/ddtrace/tracer.rb

@@ -18,6 +18,8 @@ module Datadog
  # of these function calls and sub-requests would be encapsulated within a single trace.
  # rubocop:disable Metrics/ClassLength
  class Tracer
+    DEFAULT_TIMEOUT = 5


Shall we move this one in the AsyncTransport class?

palazzem

Everything is good to me! Good job! The only thing we're missing is just a couple of comments in our new methods to describe better the API. I'd say also to add an example in the tracer.shutdown! so that people know how to use it:

# script.rb

Datadog.tracer.trace('operation_name', service='rake_tasks') do |span|
  span.set_tag('task.name', 'script')
end

Datadog.tracer.shutdown!

palazzem · 2017-09-26T13:49:08Z

lib/ddtrace/tracer.rb

@@ -57,6 +57,11 @@ def self.debug_logging
      log.level == Logger::DEBUG
    end

+    def shutdown!


👍 I'd say to write some documentation about this method:

# Shorthand that calls the `shutdown!` method of a registered worker. # It's useful to ensure that the Trace Buffer is properly flushed before # shutting down the application.

palazzem · 2017-09-26T13:53:17Z

lib/ddtrace/workers.rb

@@ -87,6 +92,20 @@ def stop
        @run = false
      end

+      def shutdown!


We should write a couple of lines to document what this method is doing. I'll let you describe this (i.e. "this method closes all available queues and wait till the trace buffer and service are flushed").

palazzem

All good to me! great job! let's apply this improvement to our Resque integration

…er is shut down. Adds tests for these changes

…sh before exiting

…eared but the traces are still being sent. Updates test to use a thread to make sure shutdown is not called more than once

palazzem

just a question about the shutdown!; if I'm not wrong, we may simplify it a bit removing all the timeout / sleep block

palazzem · 2017-09-29T20:44:46Z

lib/ddtrace/workers.rb

+        @shutting_down = true
+        @trace_buffer.close
+        @service_buffer.close
+        sleep(0.1)


do we need any sleep at all? if we close both buffers and we stop the worker, technically we should just join no? looking here:

dd-trace-rb/lib/ddtrace/workers.rb

Lines 66 to 70 in 47b0dd2

while @run

callback_traces

callback_services

sleep(@flush_interval) if @run

end

both callback_traces (and callback_services):

empties the buffer

sends the buffers

So if we join we simply wait that the flushing thread exits the while, so for sure buffers are empty AND sent. The join has a timeout of 5 seconds, that is x5 times our flushing interval so it's reasonable high to ensure that traces are flushed.

Am I missing something?

@ryplo since this approach works for Resque, we can merge the PR and make a new one to improve that code block.

palazzem

Going to merge the PR! Thanks for porting the shutdown! method!

palazzem · 2017-10-02T02:21:36Z

lib/ddtrace/workers.rb

+        @shutting_down = true
+        @trace_buffer.close
+        @service_buffer.close
+        sleep(0.1)


@ryplo since this approach works for Resque, we can merge the PR and make a new one to improve that code block.

palazzem added this to the 0.9.0 milestone Sep 22, 2017

palazzem added the core Involves Datadog core libraries label Sep 22, 2017

ryplo added the do-not-merge/WIP Not ready for merge label Sep 22, 2017

ryplo force-pushed the rachel/implement-shutdown branch from 493cc63 to 592317f Compare September 25, 2017 18:18

palazzem reviewed Sep 25, 2017

View reviewed changes

ryplo force-pushed the rachel/implement-shutdown branch 2 times, most recently from 045040d to 4cb2199 Compare September 25, 2017 21:31

palazzem removed the do-not-merge/WIP Not ready for merge label Sep 26, 2017

palazzem suggested changes Sep 26, 2017

View reviewed changes

palazzem previously approved these changes Sep 26, 2017

View reviewed changes

ryplo dismissed palazzem’s stale review via 3a65337 September 26, 2017 17:47

ryplo force-pushed the rachel/implement-shutdown branch from 3a65337 to 48708fb Compare September 27, 2017 19:29

ryplo changed the base branch from master to rachel/services-flush September 27, 2017 19:29

ryplo changed the base branch from rachel/services-flush to master September 27, 2017 21:28

ryplo force-pushed the rachel/implement-shutdown branch 3 times, most recently from f04f617 to 770366d Compare September 28, 2017 23:25

Rachel Lo added 10 commits September 29, 2017 16:15

Adds shutdown function to sleep, check for non empty buffers, repeat

30eff3d

Cleanup of shutdown method

88b47b4

Adds check to make sure shutdown only called once. More cleanup

8f8e71b

change instance var

08bd23f

Moves shutdown method and flag to the worker

6d328e9

Allows trace buffer to be closed. Makes sure it is closed before work…

948b72d

…er is shut down. Adds tests for these changes

Adds comments to the tracer and worker shutdown methods

c709b2e

Fixes test for CI

e3f20cc

Shutdown method tells worker thread to stop, and waits for it to fini…

98a826c

…sh before exiting

Fixes check for shutdown method so that traces send when buffer is cl…

47b0dd2

…eared but the traces are still being sent. Updates test to use a thread to make sure shutdown is not called more than once

ryplo force-pushed the rachel/implement-shutdown branch from 770366d to 47b0dd2 Compare September 29, 2017 20:15

ryplo added the do-not-merge/WIP Not ready for merge label Sep 29, 2017

palazzem reviewed Sep 29, 2017

View reviewed changes

palazzem removed the do-not-merge/WIP Not ready for merge label Oct 2, 2017

palazzem approved these changes Oct 2, 2017

View reviewed changes

palazzem merged commit 1a610f7 into master Oct 2, 2017

palazzem deleted the rachel/implement-shutdown branch October 2, 2017 02:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ensure traces sent before process ends #195

Ensure traces sent before process ends #195

ryplo commented Sep 22, 2017 •

edited by palazzem

Loading

palazzem Sep 25, 2017

palazzem left a comment

palazzem Sep 26, 2017

palazzem Sep 26, 2017

palazzem left a comment

palazzem left a comment

palazzem Sep 29, 2017

palazzem Oct 2, 2017

palazzem left a comment

palazzem Oct 2, 2017

	while @run
	callback_traces
	callback_services
	sleep(@flush_interval) if @run
	end

Ensure traces sent before process ends #195

Ensure traces sent before process ends #195

Conversation

ryplo commented Sep 22, 2017 • edited by palazzem Loading

Overview

Major changes

User impact

palazzem Sep 25, 2017

Choose a reason for hiding this comment

palazzem left a comment

Choose a reason for hiding this comment

palazzem Sep 26, 2017

Choose a reason for hiding this comment

palazzem Sep 26, 2017

Choose a reason for hiding this comment

palazzem left a comment

Choose a reason for hiding this comment

palazzem left a comment

Choose a reason for hiding this comment

palazzem Sep 29, 2017

Choose a reason for hiding this comment

palazzem Oct 2, 2017

Choose a reason for hiding this comment

palazzem left a comment

Choose a reason for hiding this comment

palazzem Oct 2, 2017

Choose a reason for hiding this comment

ryplo commented Sep 22, 2017 •

edited by palazzem

Loading