mu-semtech / delta-notifier

Sends notifications about changes to interested mu.semte.ch microservices
0 stars 14 forks source link

add bundling #18

Closed Rahien closed 6 months ago

Rahien commented 7 months ago

This PR adds bundling requests if a service specifies a grace-period. With debug logs on, this results in:

deltanotifier-1  | Rebuilding sources to include /config.
deltanotifier-1  | Successfully compiled 6 files with Babel (254ms).
deltanotifier-1  | Successfully compiled 3 files with Babel (261ms).
deltanotifier-1  | Starting server on 0.0.0.0:80 in production mode
deltanotifier-1  | Going to send POST to http://ldes-delta-pusher/publish
deltanotifier-1  | Creating bundle for key 1-http://mu.semte.ch/sessions/d7773ca0-dd5b-11ee-befb-0242ac120005, sending in 10000ms
deltanotifier-1  | Going to send POST to http://resource/.mu/delta
deltanotifier-1  | Creating bundle for key 0-http://mu.semte.ch/sessions/d7773ca0-dd5b-11ee-befb-0242ac120005, sending in 250ms
deltanotifier-1  | Executing send POST to http://resource/.mu/delta
deltanotifier-1  | Going to send POST to http://ldes-delta-pusher/publish
deltanotifier-1  | Adding to bundle for key 1-http://mu.semte.ch/sessions/d7773ca0-dd5b-11ee-befb-0242ac120005, now contains 3 change sets
deltanotifier-1  | Going to send POST to http://ldes-delta-pusher/publish
deltanotifier-1  | Adding to bundle for key 1-http://mu.semte.ch/sessions/d7773ca0-dd5b-11ee-befb-0242ac120005, now contains 5 change sets
deltanotifier-1  | Going to send POST to http://ldes-delta-pusher/publish
deltanotifier-1  | Adding to bundle for key 1-http://mu.semte.ch/sessions/d7773ca0-dd5b-11ee-befb-0242ac120005, now contains 7 change sets
deltanotifier-1  | Going to send POST to http://ldes-delta-pusher/publish
deltanotifier-1  | Adding to bundle for key 1-http://mu.semte.ch/sessions/d7773ca0-dd5b-11ee-befb-0242ac120005, now contains 9 change sets
deltanotifier-1  | Going to send POST to http://ldes-delta-pusher/publish
deltanotifier-1  | Adding to bundle for key 1-http://mu.semte.ch/sessions/d7773ca0-dd5b-11ee-befb-0242ac120005, now contains 11 change sets
deltanotifier-1  | Going to send POST to http://resource/.mu/delta
deltanotifier-1  | Creating bundle for key 0-http://mu.semte.ch/sessions/d7773ca0-dd5b-11ee-befb-0242ac120005, sending in 250ms
deltanotifier-1  | Executing send POST to http://resource/.mu/delta
deltanotifier-1  | Going to send POST to http://ldes-delta-pusher/publish
deltanotifier-1  | Adding to bundle for key 1-http://mu.semte.ch/sessions/d7773ca0-dd5b-11ee-befb-0242ac120005, now contains 12 change sets
deltanotifier-1  | Going to send POST to http://ldes-delta-pusher/publish
deltanotifier-1  | Adding to bundle for key 1-http://mu.semte.ch/sessions/d7773ca0-dd5b-11ee-befb-0242ac120005, now contains 14 change sets
deltanotifier-1  | Going to send POST to http://ldes-delta-pusher/publish
deltanotifier-1  | Adding to bundle for key 1-http://mu.semte.ch/sessions/d7773ca0-dd5b-11ee-befb-0242ac120005, now contains 16 change sets
deltanotifier-1  | Going to send POST to http://ldes-delta-pusher/publish
deltanotifier-1  | Adding to bundle for key 1-http://mu.semte.ch/sessions/d7773ca0-dd5b-11ee-befb-0242ac120005, now contains 18 change sets
deltanotifier-1  | Executing send POST to http://ldes-delta-pusher/publish

My service that uses the delta notifier correctly only receives one combined changeset.

The PR also includes a retry mechanism (configurable) since I found a TODO in the code for that. It also extracts the sendRequest function to a separate file to keep the app.js a little more concise.

madnificent commented 7 months ago

Could we move the behaviour on retries to a per-service configuration, perhaps keeping the environment variables as defaults? A previous implementation in which it was verified that services accepted the delta message failed because services can behave very differently.

madnificent commented 7 months ago

This implementation ignores the mu-auth-allowed-groups which I suspect will be a problem for some services and it should become an issue for more as we get to lower the mu-auth-sudo services. Do we turn off bundling for those, or should we allow to bundle requests based on the mu-auth-allowed-groups?

Rahien commented 7 months ago

This implementation ignores the mu-auth-allowed-groups which I suspect will be a problem for some services and it should become an issue for more as we get to lower the mu-auth-sudo services. Do we turn off bundling for those, or should we allow to bundle requests based on the mu-auth-allowed-groups?

I'm not sure it does (much) more than the current implementation, which also takes the first of the mu-auth-allowed-groups of the delta coming in. Since the session remains the same, it should be reasonably fine to still use the first item's mu-auth-allowed-groups (unless there is a change in the allowed groups in the bundling period, which should be very short). We could also use the allowed groups when computing the bundle key if you'd prefer?

Rahien commented 6 months ago

Could we move the behaviour on retries to a per-service configuration, perhaps keeping the environment variables as defaults? A previous implementation in which it was verified that services accepted the delta message failed because services can behave very differently.

I now made the retry and retryTimeout configurable per rule

Rahien commented 6 months ago

This implementation ignores the mu-auth-allowed-groups which I suspect will be a problem for some services and it should become an issue for more as we get to lower the mu-auth-sudo services. Do we turn off bundling for those, or should we allow to bundle requests based on the mu-auth-allowed-groups?

I now also allow for (optional) precise bundling, which takes a sha256 hash of the mu-auth-groups as part of the bundling key. It's optional because it could slow things down...

madnificent commented 6 months ago

We made a feature build of this at semtech/mu-delta-notifier:feature-bundled-requests with the currently latest code of the PR

nvdk commented 6 months ago

We should probably add a warning that delta's might be delivered out of order in case of failures to deliver a delta. as far as I can tell there's no logic to ensure correct order in case of retries