IPC based Circuit breaker allowing all currently running PHP processes to stop sending data to the agent #440

pawelchcki · 2019-05-16T10:17:49Z

Description

This also persists the circuit breaker state between restarts so that if the PHP process is recycled it will inherit the CB state.

Its using shm_open to allocate shared memory object (on linux its an actual file in /dev/shm) base on path (path is built from dd_trace_+VERSION_STRING).

Then that shared memory object is mapped to local address space and accessed locally via atomic C11 functions, guaranteeing consistency when setting /incrementing/changing a value.

See:
https://en.wikipedia.org/wiki/Circuit_breaker_design_pattern

Readiness checklist

Changelog entry added, if necessary
Tests added for this feature/bug

SammyK

Mind. Blown. Fantastic work @pawelchcki! I just left a few comments I'd love to get your thoughts on. :)

package.xml

SammyK · 2019-05-16T22:44:47Z

src/ext/circuit_breaker.c

@@ -0,0 +1,137 @@
+#include <fcntl.h>


This whole thing is 🔝! :)

SammyK · 2019-05-16T22:54:03Z

src/ext/circuit_breaker.c

+    prepare_cb();
+
+    atomic_fetch_add(&dd_trace_circuit_breaker->consecutive_failures, 1);
+    atomic_fetch_add(&dd_trace_circuit_breaker->total_failures, 1);


Can total_failures be removed? I don't see it being used anywhere else. :)

I left in in since this is the data I would love to collect with runtime metrics. But right now it doesn't serve any purpose.

However I think actually might be good idea to add debug mode, where this data could be exposed.

I ❤️ the idea of putting that at debug level. :)

SammyK · 2019-05-16T22:56:06Z

tests/ext/circuit_breaker_low_level.phpt

@@ -0,0 +1,16 @@
+--TEST--
+Test circuit_breaker functionality


Would it be possible to make each of the test names a little bit more unique? They're all the same at the moment. :)

SammyK · 2019-05-16T23:02:39Z

src/ext/vendor_stdatomic.h

@@ -0,0 +1,325 @@
+/*


Since this is the second lib we've vendored in, we should probably start considering were we put 3rd-party code. I put the MessagePack files in src/ext/mpack but I'm not 100% happy with that location. And dropping in a vendor folder could get confusing with Composer. So I'm open to ideas, but I'd love to consolidate all the vendered code. :)

Also - could we append this line to the .gitattributes to have this auto-collapse in diffs:

src/ext/vendor_stdatomic.h* linguist-generated=true

If we throw all the vendored code into one directory, we can just always have it auto-collapse every time we add/update:

src/ext/vendored_code_here/* linguist-generated=true

Anyway - I'd love to hear your thoughts. :)

Need to vendor in this file surprised me abit. Hopefully we'll be able to get rid of this once we'll be able to upgrade GCC in 7.3 build images
stdatomic.h should be part of C11 standard but GCC lagged "a bit" in implementing it :(

Basically I hope to get rid of vendor_stdatomic.h "soon" so the sorest it looks the higher chance we'll remember to remove it.

overall I don't mind having vendor folder. But I worry about being able to set include paths correctly so that the files don't need to be modified when they reference themselves under specifc paths. "Practicality beats purity" etc :)

src/DDTrace/Transport/Http.php

SammyK · 2019-05-16T23:12:55Z

src/ext/ddtrace.c

+    RETURN_BOOL(1);
+}
+
+static PHP_FUNCTION(dd_tracer_circuit_breaker_open) {


As far as I can tell, the only functions that are being used in userland are dd_tracer_circuit_breaker_can_try(), dd_tracer_circuit_breaker_register_error(), and dd_tracer_circuit_breaker_register_success(). So I'm curious if the other functions are just exposing an API for testing purposes? If so, I'm wondering if there's another solution that doesn't require adding additional functions in userland. Maybe only exposing them when compiling in debug mode or something? What are your thoughts? :)

I wonder whats the downside keeping them in?
I would prefer to test as close to the final binary as possible so functions only exposed in tests would mean that we have two different binaries - which in turn could foster theoretical edge-cases which wouldn't be detected in tests.

Also those functions provide more access to the Circuit breaker and could be used to extend/ enhance the functioning a bit in the future.

I wonder whats the downside keeping them in?

I'm just thinking of the commitment to ongoing maintenance and not exposing any API's that aren't necessary so we have fewer issues if we need to change or remove them. But I'm not saying we shouldn't add them, I'm just wondering if we have sufficient reason for adding them and are OK with any tradeoffs for adding them. So if you're confident, I'm confident too. :)

could be used to extend/ enhance the functioning a bit in the future.

I personally don't like adding code for possible future features since the domain could change and we have to commit to maintaining the future code for an indeterminate amount of time. In my experience, most times I've added future code, the future code is never used. :D

I would prefer to test as close to the final binary as possible

I agree - I think "to ensure stability" is a pretty good reason to expose these. I would prefer a method to ensure stability that doesn't expose any more public API's, but if there's really no other viable alternative, I'm on board with this! :)

Excellent points Sammy!

I think I could remove a few functions though. Will probably be able to drop _close and _open. Probably _is_closed to since its functionality is implemented by a new _info .

tests/ext/circuit_breaker_retry_time.phpt

…exceptions for PHP 5

SammyK

Lovely work @pawelchcki! :)

SammyK · 2019-05-22T15:51:03Z

src/DDTrace/Transport/Http.php

+        if ($this->isLogDebugActive() && function_exists('dd_tracer_circuit_breaker_info')) {
+            self::logDebug('circuit breaker status: closed => {closed}, total_failures => {total_failures},'
+            . 'consecutive_failures => {consecutive_failures}, opened_timestamp => {opened_timestamp}, '
+            . 'last_failure_timestamp=> {last_failure_timestamp}', dd_tracer_circuit_breaker_info());


SammyK · 2019-05-22T15:51:25Z

.gitattributes

@@ -11,3 +11,4 @@ RELEASING.md        export-ignore
 .circleci           export-ignore
 .clang-format       export-ignore
 src/ext/mpack/*     linguist-generated=true
+src/ext/vendor_stdatomic.h     linguist-generated=true


pawelchcki force-pushed the pawel/build_circuit_breaker_into_communication branch 12 times, most recently from 5d2c28c to 19613b6 Compare May 16, 2019 13:18

pawelchcki changed the title ~~WIP: IPC based Circuit breaker allowing all currently running PHP processes to stop sending data to the agent~~ IPC based Circuit breaker allowing all currently running PHP processes to stop sending data to the agent May 16, 2019

pawelchcki force-pushed the pawel/build_circuit_breaker_into_communication branch from 19613b6 to 91d3b15 Compare May 16, 2019 14:19

pawelchcki requested review from SammyK and labbati and removed request for SammyK May 16, 2019 15:22

SammyK suggested changes May 16, 2019

View reviewed changes

pawelchcki force-pushed the pawel/build_circuit_breaker_into_communication branch from 91d3b15 to 8419d5a Compare May 17, 2019 10:25

pawelchcki added this to the 0.25.0 milestone May 17, 2019

pawelchcki force-pushed the pawel/build_circuit_breaker_into_communication branch 5 times, most recently from 72c4ac0 to 81a7cae Compare May 22, 2019 12:19

pawelchcki added 5 commits May 22, 2019 16:13

Basic circuit breaker C implementation

a6a5919

vendor in stdatomic to allow comilation on older (sigh) systems

494fa01

Circuit breaker implementation and initial tests

12db454

circuit breaker max failures test

b986997

test being able to override max consecutive failures

585f2a9

pawelchcki added 8 commits May 22, 2019 16:13

verify retry time is handled correctly

4f95969

disable GCC check when initializing circuit breaker + add zts unused …

66ea81a

…exceptions for PHP 5

add -lrt to compilation

2db6850

rename circuit breaker retry to try

2de3e31

actually add circuit breaker to Http transport + tests

3583ddf

Reduce the circuit breaker tests timeouts to ~1/3

694adb3

Add dd_tracer_circuit_breaker_info debug message

76616c0

remove unnecessary low level functions functions + cleanup tests naming

bc24a8e

pawelchcki force-pushed the pawel/build_circuit_breaker_into_communication branch from 81a7cae to bc24a8e Compare May 22, 2019 14:13

SammyK approved these changes May 22, 2019

View reviewed changes

pawelchcki merged commit ba07549 into master May 22, 2019

pawelchcki deleted the pawel/build_circuit_breaker_into_communication branch May 22, 2019 16:10

pawelchcki mentioned this pull request May 27, 2019

Release 0.25.0 #453

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

IPC based Circuit breaker allowing all currently running PHP processes to stop sending data to the agent #440

IPC based Circuit breaker allowing all currently running PHP processes to stop sending data to the agent #440

pawelchcki commented May 16, 2019 •

edited

SammyK left a comment

SammyK May 16, 2019

SammyK May 16, 2019

pawelchcki May 17, 2019

SammyK May 17, 2019

SammyK May 16, 2019

SammyK May 16, 2019

pawelchcki May 17, 2019

pawelchcki May 17, 2019

SammyK May 16, 2019

pawelchcki May 17, 2019

SammyK May 17, 2019

pawelchcki May 17, 2019

SammyK left a comment

SammyK May 22, 2019

SammyK May 22, 2019

IPC based Circuit breaker allowing all currently running PHP processes to stop sending data to the agent #440

IPC based Circuit breaker allowing all currently running PHP processes to stop sending data to the agent #440

Conversation

pawelchcki commented May 16, 2019 • edited

Description

Readiness checklist

SammyK left a 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

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

SammyK left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pawelchcki commented May 16, 2019 •

edited