add integration tests for the API and the background worker by palazzem · Pull Request #95 · DataDog/dd-trace-py

palazzem · 2016-11-07T12:40:05Z

What it does

Functionalities

Add utility methods to the AsyncWorker
The Q trace buffer could be not bounded if a max_size of 0 is given (like the built-in Queue)
Move all the encoding logic inside the encoding.py

Tests

add tests for the encoders
add integration tests for the API connector
add integration tests for the AsyncWorker so that the full flow is tested (tracer.trace() calls the local trace-agent when it's time to flush).

To launch integration tests simply:

TEST_DATADOG_INTEGRATION=1 tox

What's missing

~~a Docker container that could be loaded by our CI (otherwise tests will fail)~~

…encoder

…d integration tests for Traces and Services endpoints

…n the integration tests

… for the complete tracing flow

palazzem · 2016-11-07T12:42:02Z

+    """
+    Send data to the trace agent using the HTTP protocol and JSON format
+    """
+    def __init__(self, hostname, port, wait_response=False):


I don't want to change the current behavior (ignore the response) so I added a wait_response that if set to True (it happens only for tests) will wait and get the trace agent response.

should we make this the default with a timeout?

I'm not sure. If we have an overhead here, we could impact application performance. I prefer to keep the default as "we don't wait for a response", spending some time later to do a proper application profiling. In this way we don't block this PR from being merged.

palazzem · 2016-11-07T12:42:40Z


    def send_traces(self, traces):
-        spans = [item for sublist in traces for item in sublist]
-        self.send_spans(spans)


I moved all the serialization logic into the encoding.py

palazzem · 2016-11-07T12:43:23Z


-def encode_spans(spans):
-    return json.dumps([s.to_dict() for s in spans])
+def encode_spans(traces):


Only this function will change after migrating to the new v0.2 API

palazzem · 2016-11-07T12:44:36Z

+        """
+        with self._lock:
+            if self._thread and self.is_alive():
+                self._trace_queue.close()


I think this is the termination clause.

palazzem · 2016-11-07T12:45:29Z

                return False

-            if len(self._things) < self._max_size:
+            if len(self._things) < self._max_size or self._max_size <= 0:


Just because it's the default behavior of a built-in Queue.

palazzem · 2016-11-07T12:46:05Z

+    os.environ.get('TEST_DATADOG_INTEGRATION', False),
+    'You should have a running trace agent and set TEST_DATADOG_INTEGRATION=1 env variable'
+)
+class TestWorkers(TestCase):


Tests the full flow (but not if the trace agent accepts our payload). If one of these tests fails, it means that we're moving traces and services in a wrong way between the Tracer, the Writer and the AsyncWorker.

palazzem · 2016-11-07T12:46:22Z

+    os.environ.get('TEST_DATADOG_INTEGRATION', False),
+    'You should have a running trace agent and set TEST_DATADOG_INTEGRATION=1 env variable'
+)
+class TestAPITransport(TestCase):


Tests if the trace agent accepts our payload.

palazzem · 2016-11-07T12:46:40Z

@@ -1,15 +0,0 @@
-
-
-from ddtrace import writer


moved in the buffer tests

palazzem · 2016-11-07T12:47:08Z

-
-        # encode so things work.
        if spans:
-            encoding.encode_spans(spans)


I think we don't need this anymore, because we're already testing the encoding in our integration tests.

we should keep it because it checks all integrations by default. it's a very cheap safety net to have and has caught bugs in the past.

So I have to make some changes because we're not encoding traces here, only a single list of spans.

clutchski · 2016-11-07T16:07:42Z

+    :param traces: A list of traces that should be serialized
+    """
+    flatten_spans = [span.to_dict() for trace in traces for span in trace]
+    return json.dumps(flatten_spans)


can we have two functions here? encode_dpans and encode traces? so that it's clear what the diff is?

should we keep one encoding function for traces that calls the other encoding function for spans? is it really required by the API class? Anyway, I put everything as a one-liner just because in the next API version (v0.2) we should not flatten anymore the traces list.

Not sure that it's really important keeping two different functions (considered that the list comprehension is faster if we use it to transform and append many items). @clutchski is your request only related to make this part more readable than two for nesting in the list comprehension?

clutchski · 2016-11-07T16:08:41Z

+from ddtrace.tracer import Tracer
+
+
+@skipUnless(


move this to setup? and break it up on a sane number of lines?

what do you mean by moving the skip in the setup? the entire class should be skipped if the environment variable is not set.

clutchski · 2016-11-07T16:09:22Z


+        # read the server response only if the
+        # API object is configured to do so
+        if self._wait_response:


should this be the defeault? should we add a timeout here?

Actually a real client execution should never enter here (at least for the current implementation) and it should be used only for testing purposes. About the timeout I have to check if it's really required for the getresponse() since we're using it only on tests.

I really prefer that the default behavior is to not read the response and relying only on HTTP exceptions to handle errors.

answered in the comment above

clutchski · 2016-11-07T16:48:52Z


 # project
 import ddtrace.encoding



nit: remove blankline

clutchski · 2016-11-07T16:49:39Z

+        Wait for the AsyncWorker execution. This call is not blocking and has
+        a timeout of 2 seconds.
+        """
+        self._thread.join(timeout=2)


make the timeout an arg?

clutchski · 2016-11-07T16:50:39Z

+from nose.tools import eq_, ok_

+from ddtrace.span import Span
+from ddtrace.writer import Q as TraceBuffer


why? :) not worth. can you merge these tests with the ones in test_writer?

Just to have a meaningful name here that we have another kind of buffer (and I didn't want to use Queue). About the test to merge do you mean -> https://github.com/DataDog/dd-trace-py/pull/95/files#diff-240fdd5fbba45c9eb1bc0402cebbfb8aL5 ?

The previous test was merged and I kept Q as a name.

clutchski · 2016-11-07T16:51:02Z

+
+        response = self.api.send_traces(traces)
+        ok_(response)
+        eq_(response.status, 200)


cool this is great!

clutchski · 2016-11-07T16:52:12Z

some nits but looks good

palazzem · 2016-11-08T09:29:58Z

-            encoding.encode_spans(spans)
+            # the traces encoding expect a list of traces so we
+            # put spans in a list like we do in the real execution path
+            encoding.encode_traces([spans])


had to change that in [spans] instead of spans because this is the right payload that we expect for the encoding function (we always get a list of traces from the Q in the real execution path)

LeoCavaille

LGTM. We should send traces and clean that legacy serialized format but we can keep that for another PR.

palazzem · 2016-11-08T16:59:29Z

Cool, fixing encoding stuff (and endpoint) in a new PR.

…llocator (#17664) ## Description This PR fixes a segmentation fault in the memory allocation profiler that occurs when a hook call races with `memalloc` start/stop operations. The issue arises from concurrent access to the saved allocator struct, which could be partially written while being read, resulting in`NULL` function pointers being dereferenced. The key indicator in that case is that `#1 0x0000000000000000` frame -- we are trying to execute a null function pointer. ```` Error UnixSignal: Process terminated with SEGV_MAPERR (SIGSEGV) #0 0x00007ff3c303a8d4 #1 0x0000000000000000 memalloc_alloc (/go/src/github.com/DataDog/apm-reliability/dd-trace-py/ddtrace/profiling/collector/_memalloc.cpp:68) #2 0x00007ff39dcb3b20 memalloc_alloc (/go/src/github.com/DataDog/apm-reliability/dd-trace-py/ddtrace/profiling/collector/_memalloc.cpp:68) #3 0x00007ff39dcb3b20 memalloc_malloc(void*, unsigned long) (/go/src/github.com/DataDog/apm-reliability/dd-trace-py/ddtrace/profiling/collector/_memalloc.cpp:80) #4 0x00007ff3c3087e1b PyUnicode_New #5 0x00007ff3c30889f4 #6 0x00007ff3c3170c84 #7 0x00007ff3c316b931 #8 0x00007ff3c31aaac8 #9 0x00007ff3c31033ac #10 0x00007ff3c310e2a6 PyObject_CallMethodObjArgs #11 0x00007ff3c310e46d #12 0x00007ff3c31a96c2 #13 0x00007ff3c3102fd7 PyObject_Vectorcall #14 0x00007ff3c32335a2 _PyEval_EvalFrameDefault #15 0x00007ff3c323c094 #16 0x00007ff3c3233dd3 _PyEval_EvalFrameDefault #17 0x00007ff3c323c094 #18 0x00007ff3c30e997d PyObject_CallOneArg #19 0x00007ff3c306a480 _PyObject_GenericGetAttrWithDict #20 0x00007ff3c30c620d PyObject_GetAttr #21 0x00007ff3c32309e7 _PyEval_EvalFrameDefault #22 0x00007ff3c323c094 #23 0x00007ff3c312880e #24 0x00007ff3c30e917c _PyObject_MakeTpCall #25 0x00007ff3c32335a2 _PyEval_EvalFrameDefault #26 0x00007ff3c323c094 #27 0x00007ff3c312880e #28 0x00007ff3c30e917c _PyObject_MakeTpCall #29 0x00007ff3c32335a2 _PyEval_EvalFrameDefault #30 0x00007ff3c323c094 #31 0x00007ff3c3233dd3 _PyEval_EvalFrameDefault #32 0x00007ff3c323c094 #33 0x00007ff3c317d0fd #34 0x00007ff3c3233dd3 _PyEval_EvalFrameDefault #35 0x00007ff3c323c094 #36 0x00007ff3c3233dd3 _PyEval_EvalFrameDefault #37 0x00007ff3c323c094 #38 0x00007ff3c317d1b5 #39 0x00007ff3c3102fd7 PyObject_Vectorcall #40 0x00007ff3c3232f4a _PyEval_EvalFrameDefault #41 0x00007ff3c3240da5 #42 0x00007ff3c324112d #43 0x00007ff3c3233be1 _PyEval_EvalFrameDefault #44 0x00007ff3c323c094 #45 0x00007ff3c317d1b5 #46 0x00007ff3c3102fd7 PyObject_Vectorcall #47 0x00007ff3c3232f4a _PyEval_EvalFrameDefault #48 0x00007ff3c323c094 #49 0x00007ff3c31033ac #50 0x00007ff3c310358d PyObject_CallFunctionObjArgs #51 0x00007ff3bf7eb91d WraptBoundFunctionWrapper_call (/project/src/wrapt/_wrappers.c:3750) #52 0x00007ff3c3104055 _PyObject_Call #53 0x00007ff3c3233dd3 _PyEval_EvalFrameDefault #54 0x00007ff3c323c094 #55 0x00007ff3c317d23c #56 0x00007ff3c3233dd3 _PyEval_EvalFrameDefault #57 0x00007ff3c323c094 #58 0x00007ff3c310416f _PyObject_Call #59 0x00007ff3c3233dd3 _PyEval_EvalFrameDefault #60 0x00007ff3c3240da5 #61 0x00007ff3c324112d #62 0x00007ff3c3233be1 _PyEval_EvalFrameDefault #63 0x00007ff3c323c094 #64 0x00007ff3c317d1b5 #65 0x00007ff3c3102fd7 PyObject_Vectorcall #66 0x00007ff3c3232f4a _PyEval_EvalFrameDefault #67 0x00007ff3c323c094 #68 0x00007ff3c317d1b5 #69 0x00007ff3c310416f _PyObject_Call #70 0x00007ff3c3233dd3 _PyEval_EvalFrameDefault #71 0x00007ff3c323c094 #72 0x00007ff3c317d1b5 #73 0x00007ff3c310416f _PyObject_Call #74 0x00007ff3c3233dd3 _PyEval_EvalFrameDefault #75 0x00007ff3c323c094 #76 0x00007ff3c31033ac #77 0x00007ff3c310358d PyObject_CallFunctionObjArgs #78 0x00007ff3bf7eb91d WraptBoundFunctionWrapper_call (/project/src/wrapt/_wrappers.c:3750) #79 0x00007ff3c30e917c _PyObject_MakeTpCall #80 0x00007ff3c32335a2 _PyEval_EvalFrameDefault #81 0x00007ff3c323c094 #82 0x00007ff3c317d518 #83 0x00007ff3c3155963 #84 0x00007ff3c315393d #85 0x00007ff3c3233dd3 _PyEval_EvalFrameDefault #86 0x00007ff3c323c094 #87 0x00007ff3c317d0fd #88 0x00007ff3c3233dd3 _PyEval_EvalFrameDefault #89 0x00007ff3c323c094 #90 0x00007ff3c317d0fd #91 0x00007ff3c317d518 #92 0x00007ff3c3233dd3 _PyEval_EvalFrameDefault #93 0x00007ff3c323c094 #94 0x00007ff3c30e9371 _PyObject_FastCallDictTstate #95 0x00007ff3c30e958d _PyObject_Call_Prepend #96 0x00007ff3c3109150 #97 0x00007ff3c3104055 _PyObject_Call #98 0x00007ff3c3233dd3 _PyEval_EvalFrameDefault #99 0x00007ff3c323c094 #100 0x00007ff3c30e92f1 _PyObject_FastCallDictTstate #101 0x00007ff3c30e958d _PyObject_Call_Prepend #102 0x00007ff3c3109150 #103 0x00007ff3c30e917c _PyObject_MakeTpCall #104 0x00007ff3c32335a2 _PyEval_EvalFrameDefault #105 0x00007ff3c323c094 #106 0x00007ff3c30e92f1 _PyObject_FastCallDictTstate #107 0x00007ff3c30e958d _PyObject_Call_Prepend #108 0x00007ff3c3109150 #109 0x00007ff3c30e917c _PyObject_MakeTpCall #110 0x00007ff3c32335a2 _PyEval_EvalFrameDefault #111 0x00007ff3c323c094 #112 0x00007ff3c30e92f1 _PyObject_FastCallDictTstate #113 0x00007ff3c30e958d _PyObject_Call_Prepend #114 0x00007ff3c3109150 #115 0x00007ff3c30e917c _PyObject_MakeTpCall #116 0x00007ff3c32335a2 _PyEval_EvalFrameDefault #117 0x00007ff3c323c094 #118 0x00007ff3c30e92f1 _PyObject_FastCallDictTstate #119 0x00007ff3c30e958d _PyObject_Call_Prepend #120 0x00007ff3c3109150 #121 0x00007ff3c30e917c _PyObject_MakeTpCall #122 0x00007ff3c32335a2 _PyEval_EvalFrameDefault #123 0x00007ff3c323c094 #124 0x00007ff3c30e92f1 _PyObject_FastCallDictTstate #125 0x00007ff3c30e958d _PyObject_Call_Prepend #126 0x00007ff3c3109150 #127 0x00007ff3c30e917c _PyObject_MakeTpCall #128 0x00007ff3c32335a2 _PyEval_EvalFrameDefault #129 0x00007ff3c323c094 #130 0x00007ff3c30e92f1 _PyObject_FastCallDictTstate #131 0x00007ff3c30e958d _PyObject_Call_Prepend #132 0x00007ff3c3109150 #133 0x00007ff3c30e917c _PyObject_MakeTpCall #134 0x00007ff3c32335a2 _PyEval_EvalFrameDefault #135 0x00007ff3c323c094 #136 0x00007ff3c30e92f1 _PyObject_FastCallDictTstate #137 0x00007ff3c30e958d _PyObject_Call_Prepend #138 0x00007ff3c3109150 #139 0x00007ff3c30e917c _PyObject_MakeTpCall #140 0x00007ff3c32335a2 _PyEval_EvalFrameDefault #141 0x00007ff3c323c094 #142 0x00007ff3c30e92f1 _PyObject_FastCallDictTstate #143 0x00007ff3c30e958d _PyObject_Call_Prepend #144 0x00007ff3c3109150 #145 0x00007ff3c30e917c _PyObject_MakeTpCall #146 0x00007ff3c32335a2 _PyEval_EvalFrameDefault #147 0x00007ff3c323c094 #148 0x00007ff3c30e92f1 _PyObject_FastCallDictTstate #149 0x00007ff3c30e958d _PyObject_Call_Prepend #150 0x00007ff3c3109150 #151 0x00007ff3c30e917c _PyObject_MakeTpCall #152 0x00007ff3c32335a2 _PyEval_EvalFrameDefault #153 0x00007ff3c323c094 #154 0x00007ff3c30e92f1 _PyObject_FastCallDictTstate #155 0x00007ff3c30e958d _PyObject_Call_Prepend #156 0x00007ff3c3109150 #157 0x00007ff3c30e917c _PyObject_MakeTpCall #158 0x00007ff3c32335a2 _PyEval_EvalFrameDefault #159 0x00007ff3c323c094 #160 0x00007ff3c30e92f1 _PyObject_FastCallDictTstate #161 0x00007ff3c30e958d _PyObject_Call_Prepend #162 0x00007ff3c3109150 #163 0x00007ff3c30e917c _PyObject_MakeTpCall #164 0x00007ff3c32335a2 _PyEval_EvalFrameDefault #165 0x00007ff3c323c094 #166 0x00007ff3c30e92f1 _PyObject_FastCallDictTstate #167 0x00007ff3c30e958d _PyObject_Call_Prepend #168 0x00007ff3c3109150 #169 0x00007ff3c30e917c _PyObject_MakeTpCall #170 0x00007ff3c32335a2 _PyEval_EvalFrameDefault #171 0x00007ff3c323c094 #172 0x00007ff3c30e92f1 _PyObject_FastCallDictTstate #173 0x00007ff3c30e958d _PyObject_Call_Prepend #174 0x00007ff3c3109150 #175 0x00007ff3c30e917c _PyObject_MakeTpCall #176 0x00007ff3c32335a2 _PyEval_EvalFrameDefault #177 0x00007ff3c323c094 #178 0x00007ff3c30e92f1 _PyObject_FastCallDictTstate #179 0x00007ff3c30e958d _PyObject_Call_Prepend #180 0x00007ff3c3109150 #181 0x00007ff3c30e917c _PyObject_MakeTpCall #182 0x00007ff3c32335a2 _PyEval_EvalFrameDefault #183 0x00007ff3c323c094 #184 0x00007ff3c30e92f1 _PyObject_FastCallDictTstate #185 0x00007ff3c30e958d _PyObject_Call_Prepend #186 0x00007ff3c3109150 #187 0x00007ff3c30e917c _PyObject_MakeTpCall #188 0x00007ff3c32335a2 _PyEval_EvalFrameDefault #189 0x00007ff3c323c094 #190 0x00007ff3c3233dd3 _PyEval_EvalFrameDefault #191 0x00007ff3c323c094 #192 0x00007ff3c30e92f1 _PyObject_FastCallDictTstate #193 0x00007ff3c30e958d _PyObject_Call_Prepend #194 0x00007ff3c3109150 #195 0x00007ff3c30e917c _PyObject_MakeTpCall #196 0x00007ff3c32335a2 _PyEval_EvalFrameDefault #197 0x00007ff3c323c094 #198 0x00007ff3c317d0fd #199 0x00007ff3c3233dd3 _PyEval_EvalFrameDefault #200 0x00007ff3c323c094 #201 0x00007ff3c3233dd3 _PyEval_EvalFrameDefault #202 0x00007ff3c323c094 #203 0x00007ff3c3233dd3 _PyEval_EvalFrameDefault #204 0x00007ff3c323c094 #205 0x00007ff3c3233dd3 _PyEval_EvalFrameDefault #206 0x00007ff3c323c094 #207 0x00007ff3c317d23c #208 0x00007ff3c31a7ec5 #209 0x00007ff3c301ac77 #210 0x00007ff3c357c573 ```` The fix implements two key changes. 1. **Hook functions (`memalloc_alloc`, `memalloc_realloc`)**: Snapshot the allocator struct locally before use and guard indirect function calls with `NULL` checks. This prevents crashes if a partially-written struct is observed during a start/stop race. 2. **Start/stop operations (`memalloc_start`, `memalloc_stop`)**: Use local variables and single assignments when publishing the allocator struct to `global_memalloc_ctx.pymem_allocator_obj`. This ensures concurrent hook calls observe either the old or new struct, never a partially-written intermediate state. The real root cause is that `PyMem_GetAllocator` is not documented as atomic, and the struct could be read field-by-field while being written to concurrently. By using local copies and single assignments, we ensure atomicity at the C level and prevent observation of inconsistent state. Co-authored-by: thomas.kowalski <thomas.kowalski@datadoghq.com>

Emanuele Palazzetti added 5 commits November 7, 2016 10:57

[tracer] add Q tests

d89c975

[tracer] the encoder handles the serialization; add tests for traces …

6a86c52

…encoder

[tracer] API transport can read a response if configured to do so; ad…

480dfbc

…d integration tests for Traces and Services endpoints

[tracer] remove encoding from the DummyWriter; it should be checked i…

1675e02

…n the integration tests

[tracer] AsyncWorker exposes stop() and join(); add integration tests…

5b523dc

… for the complete tracing flow

palazzem added enhancement ci labels Nov 7, 2016

palazzem commented Nov 7, 2016

View reviewed changes

Comment thread tests/test_writer.py

@@ -1,15 +0,0 @@

from ddtrace import writer

palazzem Nov 7, 2016

Copy link
Copy Markdown

Author

Choose a reason for hiding this comment

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

moved in the buffer tests

palazzem commented Nov 7, 2016

View reviewed changes

Emanuele Palazzetti added 2 commits November 7, 2016 14:14

[tracer] TestWorkers checks the sent payload

eb568c0

[ci] build the trace-agent container

de93a29

palazzem force-pushed the palazzem/integration-tests branch from 52670af to de93a29 Compare November 7, 2016 14:36

clutchski reviewed Nov 7, 2016

View reviewed changes

clutchski closed this Nov 7, 2016

clutchski reopened this Nov 7, 2016

[tracer] AsyncWorker join() timeout is an argument

2a1870d

Emanuele Palazzetti added 3 commits November 8, 2016 10:12

[tracer] minor on buffers tests

844f3f6

[tracer] renaming encode_spans in encode_traces

a7c6e2c

[tracer] re-add the encoding step in DummyWriter.write()

783ec58

palazzem commented Nov 8, 2016

View reviewed changes

LeoCavaille reviewed Nov 8, 2016

View reviewed changes

palazzem merged commit 9c9ec0e into master Nov 8, 2016

palazzem deleted the palazzem/integration-tests branch November 8, 2016 16:59

Conversation

palazzem commented Nov 7, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What it does

Functionalities

Tests

What's missing

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

palazzem Nov 7, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

palazzem Nov 7, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

palazzem Nov 7, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

palazzem Nov 7, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

clutchski commented Nov 7, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

palazzem commented Nov 7, 2016 •

edited

Loading

palazzem Nov 7, 2016 •

edited

Loading

palazzem Nov 7, 2016 •

edited

Loading

palazzem Nov 7, 2016 •

edited

Loading

palazzem Nov 7, 2016 •

edited

Loading