-
Notifications
You must be signed in to change notification settings - Fork 309
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
Allow Remote Config acknowledgements to be async #4638
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
Overall package sizeSelf size: 7.04 MB Dependency sizes| name | version | self size | total size | |------|---------|-----------|------------| | @datadog/native-appsec | 8.1.1 | 18.67 MB | 18.68 MB | | @datadog/native-iast-taint-tracking | 3.1.0 | 12.27 MB | 12.28 MB | | @datadog/pprof | 5.3.0 | 9.85 MB | 10.22 MB | | protobufjs | 7.2.5 | 2.77 MB | 5.16 MB | | @datadog/native-iast-rewriter | 2.4.1 | 2.14 MB | 2.23 MB | | @opentelemetry/core | 1.14.0 | 872.87 kB | 1.47 MB | | @datadog/native-metrics | 2.0.0 | 898.77 kB | 1.3 MB | | @opentelemetry/api | 1.8.0 | 1.21 MB | 1.21 MB | | jsonpath-plus | 9.0.0 | 580.4 kB | 1.03 MB | | import-in-the-middle | 1.8.1 | 71.67 kB | 785.15 kB | | msgpack-lite | 0.1.26 | 201.16 kB | 281.59 kB | | opentracing | 0.14.7 | 194.81 kB | 194.81 kB | | pprof-format | 2.1.0 | 111.69 kB | 111.69 kB | | @datadog/sketches-js | 2.1.0 | 109.9 kB | 109.9 kB | | semver | 7.6.3 | 95.82 kB | 95.82 kB | | lodash.sortby | 4.7.0 | 75.76 kB | 75.76 kB | | lru-cache | 7.14.0 | 74.95 kB | 74.95 kB | | ignore | 5.3.1 | 51.46 kB | 51.46 kB | | int64-buffer | 0.1.10 | 49.18 kB | 49.18 kB | | shell-quote | 1.8.1 | 44.96 kB | 44.96 kB | | istanbul-lib-coverage | 3.2.0 | 29.34 kB | 29.34 kB | | rfdc | 1.3.1 | 25.21 kB | 25.21 kB | | tlhunter-sorted-set | 0.1.0 | 24.94 kB | 24.94 kB | | limiter | 1.1.5 | 23.17 kB | 23.17 kB | | dc-polyfill | 0.1.4 | 23.1 kB | 23.1 kB | | retry | 0.13.1 | 18.85 kB | 18.85 kB | | jest-docblock | 29.7.0 | 8.99 kB | 12.76 kB | | crypto-randomuuid | 1.0.0 | 11.18 kB | 11.18 kB | | path-to-regexp | 0.1.7 | 6.78 kB | 6.78 kB | | koalas | 1.0.2 | 6.47 kB | 6.47 kB | | module-details-from-path | 1.0.3 | 4.47 kB | 4.47 kB |🤖 This report was automatically generated by heaviest-objects-in-the-universe |
BenchmarksBenchmark execution time: 2024-09-04 11:05:41 Comparing candidate commit ce21573 in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 260 metrics, 6 unstable metrics. |
f8bd87f
to
6e19c37
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #4638 +/- ##
==========================================
+ Coverage 69.19% 77.55% +8.35%
==========================================
Files 1 13 +12
Lines 198 980 +782
Branches 33 33
==========================================
+ Hits 137 760 +623
- Misses 61 220 +159 ☔ View full report in Codecov by Sentry. |
502ac79
to
2f52be1
Compare
Co-authored-by: simon-id <[email protected]>
e41ea8c
to
ce21573
Compare
Remote Config updates coming from the Agent needs to be acknowledged. This PR changes the existing behavior by optionally allowing the acknowledgement to be async. This is achieved by: - Changing the way you register products from using the `EventEmitter` API to a custom API that only allows a single handler per product (event emitters can have more than one listener, which is problematic as we can't support more than one of the listeners to acknowledge the config). The new API is as follows: - `rc.setProductHandler(product, handler)` (prevously `rc.on(product, handler)`) - `rc.removeProductHandler(product)` (prevously `rc.off(product, handler)`) - The new product handler is called similar to the old event listener, with the following exception: A new optional 4th argument is supplied called `ack`. This is a callback which can be called (sync or async) either without any arguments (to set the state to `ACKNOWLEDGED`) or with a single error argument (to set the state to `ERROR`). - If the handler function signature takes less than 4 arguments or doesn't use the rest operator (`...args`), and... - ...the handler doesn't return a `Promise`: The state is set to `ACKNOWLEDGED` right away without the handler having to do anything (existing behavior) - ...the handler returns a `Promise`, we wait until the promise is resolved or rejected and set the state accordingly (new behavior) - If the handler function signature takes 4 or more arguments or use the rest operator (`...args`): The state is left as `UNACKNOWLEDGED` until the `ack` callback is called (new behavior) - In any case, the state is still set to `ERROR` if the handler throws (existing behavior) The `RemoteConfigManager` is still an `EventEmitter` however because the `kPreUpdate` symbol is still being emitted.
Remote Config updates coming from the Agent needs to be acknowledged. This PR changes the existing behavior by optionally allowing the acknowledgement to be async. This is achieved by: - Changing the way you register products from using the `EventEmitter` API to a custom API that only allows a single handler per product (event emitters can have more than one listener, which is problematic as we can't support more than one of the listeners to acknowledge the config). The new API is as follows: - `rc.setProductHandler(product, handler)` (prevously `rc.on(product, handler)`) - `rc.removeProductHandler(product)` (prevously `rc.off(product, handler)`) - The new product handler is called similar to the old event listener, with the following exception: A new optional 4th argument is supplied called `ack`. This is a callback which can be called (sync or async) either without any arguments (to set the state to `ACKNOWLEDGED`) or with a single error argument (to set the state to `ERROR`). - If the handler function signature takes less than 4 arguments or doesn't use the rest operator (`...args`), and... - ...the handler doesn't return a `Promise`: The state is set to `ACKNOWLEDGED` right away without the handler having to do anything (existing behavior) - ...the handler returns a `Promise`, we wait until the promise is resolved or rejected and set the state accordingly (new behavior) - If the handler function signature takes 4 or more arguments or use the rest operator (`...args`): The state is left as `UNACKNOWLEDGED` until the `ack` callback is called (new behavior) - In any case, the state is still set to `ERROR` if the handler throws (existing behavior) The `RemoteConfigManager` is still an `EventEmitter` however because the `kPreUpdate` symbol is still being emitted.
Remote Config updates coming from the Agent needs to be acknowledged. This PR changes the existing behavior by optionally allowing the acknowledgement to be async. This is achieved by: - Changing the way you register products from using the `EventEmitter` API to a custom API that only allows a single handler per product (event emitters can have more than one listener, which is problematic as we can't support more than one of the listeners to acknowledge the config). The new API is as follows: - `rc.setProductHandler(product, handler)` (prevously `rc.on(product, handler)`) - `rc.removeProductHandler(product)` (prevously `rc.off(product, handler)`) - The new product handler is called similar to the old event listener, with the following exception: A new optional 4th argument is supplied called `ack`. This is a callback which can be called (sync or async) either without any arguments (to set the state to `ACKNOWLEDGED`) or with a single error argument (to set the state to `ERROR`). - If the handler function signature takes less than 4 arguments or doesn't use the rest operator (`...args`), and... - ...the handler doesn't return a `Promise`: The state is set to `ACKNOWLEDGED` right away without the handler having to do anything (existing behavior) - ...the handler returns a `Promise`, we wait until the promise is resolved or rejected and set the state accordingly (new behavior) - If the handler function signature takes 4 or more arguments or use the rest operator (`...args`): The state is left as `UNACKNOWLEDGED` until the `ack` callback is called (new behavior) - In any case, the state is still set to `ERROR` if the handler throws (existing behavior) The `RemoteConfigManager` is still an `EventEmitter` however because the `kPreUpdate` symbol is still being emitted.
Remote Config updates coming from the Agent needs to be acknowledged. This PR changes the existing behavior by optionally allowing the acknowledgement to be async. This is achieved by: - Changing the way you register products from using the `EventEmitter` API to a custom API that only allows a single handler per product (event emitters can have more than one listener, which is problematic as we can't support more than one of the listeners to acknowledge the config). The new API is as follows: - `rc.setProductHandler(product, handler)` (prevously `rc.on(product, handler)`) - `rc.removeProductHandler(product)` (prevously `rc.off(product, handler)`) - The new product handler is called similar to the old event listener, with the following exception: A new optional 4th argument is supplied called `ack`. This is a callback which can be called (sync or async) either without any arguments (to set the state to `ACKNOWLEDGED`) or with a single error argument (to set the state to `ERROR`). - If the handler function signature takes less than 4 arguments or doesn't use the rest operator (`...args`), and... - ...the handler doesn't return a `Promise`: The state is set to `ACKNOWLEDGED` right away without the handler having to do anything (existing behavior) - ...the handler returns a `Promise`, we wait until the promise is resolved or rejected and set the state accordingly (new behavior) - If the handler function signature takes 4 or more arguments or use the rest operator (`...args`): The state is left as `UNACKNOWLEDGED` until the `ack` callback is called (new behavior) - In any case, the state is still set to `ERROR` if the handler throws (existing behavior) The `RemoteConfigManager` is still an `EventEmitter` however because the `kPreUpdate` symbol is still being emitted.
Old API
Remote Config updates coming from the Agent needs to be acknowledged. Currently this is done synchronously in the Node.js tracer:
If a listener is attached to a given product, any config updates to that product is automatically acknowledged (if the listener doesn't throw, the state is set to
ACKNOWLEDGED
and if it throws, it's set toERROR
).However, just because there's a listener and it doesn't throw, doesn't mean that the received config can be processed by the listener. It could be that the listener needs to do some async things before it can determine if the received config is valid.
New API
This PR changes the existing behavior by making the acknowledgement async: A 4th argument is emitted to listeners ofrc.on(product)
which is anack
callback which should be called once the listener wants to acknowledge the received config. It can optionally be called with an error object as its only argument to set the state toERROR
instead ofACKNOWLEDGED
.Update:
This PR changes the existing behavior by optionally allowing the acknowledgement to be async. This is achieved by:
EventEmitter
API to a custom API that only allows a single handler per product (event emitters can have more than one listener, which is problematic as we can't support more than one of the listeners to acknowledge the config). The new API is as follows:rc.setProductHandler(product, handler)
(prevouslyrc.on(product, handler)
)rc.removeProductHandler(product)
(prevouslyrc.off(product, handler)
)ack
. This is a callback which can be called (sync or async) either without any arguments (to set the state toACKNOWLEDGED
) or with a single error argument (to set the state toERROR
)....args
), and...Promise
: The state is set toACKNOWLEDGED
right away without the handler having to do anything (existing behavior)Promise
, we wait until the promise is resolved or rejected and set the state accordingly (new behavior)...args
): The state is left asUNACKNOWLEDGED
until theack
callback is called (new behavior)ERROR
if the handler throws (existing behavior)The
RemoteConfigManager
is still anEventEmitter
however because thekPreUpdate
symbol is still being emitted. That part could do with a complete refactor, but is out of scope for this PR.