feat(events): Allow attaching context to event dispatching #205

felipecsl · 2025-02-10T18:52:19Z

Description

eppo-multiplatform port of JS sdk implementation Eppo-exp/js-sdk-common#213

Testing

Wrote tests

changeset-bot · 2025-02-10T18:52:23Z

🦋 Changeset detected

Latest commit: 1e17dea

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages

Name	Type
eppo_core	Patch
ruby-sdk	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

rasendubi

There seems to be a bug in implementation (haven't verified but very likely)

But overall, I would prefer moving event ingestion context to initialization parameters. I believe this makes our API less error-prone and easier to understand. It also simplifies our implementation. And I don't think this excludes any real use cases (and given that context is applied retroactively, any dynamic use case is likely to be buggy)

.changeset/shy-countries-press.md

eppo_core/src/event_ingestion/event_delivery.rs

rasendubi · 2025-02-11T14:44:29Z

ruby-sdk/lib/eppo_client/client.rb

+    # Unstable
+    # Sets an arbitrary context key/value pair to be sent with all events.
+    def unstable_set_context(key, value)
+      @core.set_context(key, value)
+    end


This API seems confusing because the name is not related to event ingestion in any way.

I have also have questions about use case and usage pattern:

I imagine that context might often be set in bulk, so maybe passing a map is more ergonomic than setting keys one by one

How to unset a key?

Does it really need to be dynamic? Given there's some lag between submitting an event and it going out to event ingestion service, setting context may apply retroactively and "reset" context for previously tracked events. I don't think this behavior is easily explainable and users are likely not expecting this, so that just seems like a trap. What do you think about making context an initialization parameter instead of new methods?

I think the plan is to allow providing a context in bulk during SDK initialization as well, that will come later

Set it to null

Yeah I think we're OK with that, as long as it's documented.

Makes sense. I think, JS implementation doesn't remove the field though but sends null

Do you mean we're good with users shooting themselves in the foot as long as it's documented behavior? 😅

point taken. I don't expect that to be a common pattern - I expect most customers will set the context once during initialization/page load, etc. or rather infrequently as a result of user actions. while I agree that there is risk of a race condition here, I'd expect it to be rare enough to be negligible. @petzel let us know what your thoughts are here, I think @rasendubi has a valid concern, which is also present in the js sdk

yup, that sounds about right. I'll make a note to clean that up

Depends on the environment but environments that go offline (e.g., mobile, web on mobile networks) or trigger retries may find the context to be off quite often. Context may also change between retries, so the server might receive duplicate events but with different context. This becomes even worse with persistence

Imo, any changeable context should be part of events themselves

we could associate a timestamp with a context and use that for ensuring they don't get incorrectly associated with events tracked before that timestamp. should be fairly simple to do with simple sorting.

we could associate a timestamp with a context and use that for ensuring they don't get incorrectly associated with events tracked before that timestamp. should be fairly simple to do with simple sorting.

That kinda works except when it doesn't. Timestamps are not monotonic and travel back in time all the time because of skews and time corrections. Timestamps are not reliable for establishing ordering/correlation of events — especially on random PCs where we don't control system time

We can establish order by using a single channel for both context and events. There's still a lot of edge cases though, and making it work with persistence is going to be painful. We can make it work reliably but I really don't think that we should — that just seems like a time sink with no real benefit.

I still don't see a reason to make context dynamic, so I would rather make it static/init-only until we have a customer with a real use case.

If we need dynamic context, the easiest solution is to attach context to each event individually. This is simple, reliable, and unlocks more use cases in the future (e.g., scoping context to a particular request in server env or integration with server overrides). It may seem wasteful duplicating the same context but HTTP body compression could really shine here and significantly reduce the overhead (and given we're sending tons of jsons and they compress really well, we should use some compression to get that 5x reduction in size)

ruby-sdk/ext/eppo_client/src/client.rs

eppo_core/src/event_ingestion/event_ingestion.rs

felipecsl · 2025-02-11T18:42:38Z

eppo_core/src/event_ingestion/event_ingestion.rs

@@ -60,17 +61,30 @@ impl EventIngestionConfig {
 /// A handle to Event Ingestion subsystem.
 pub struct EventIngestion {
    tx: mpsc::Sender<BatchedMessage<Event>>,
+    context_sender: mpsc::Sender<(String, Value)>,


I'd propose we rename tx to event_sender for symmetry and readability

eppo_core/src/event_ingestion/delivery.rs

rasendubi · 2025-02-12T23:10:07Z

eppo_core/src/event_ingestion/context.rs

+    String(Str),
+    Number(f64),
+    Boolean(bool),
+    Null,


minor: I think we want to model a potentially absent value as Option<ContextValue>. This will make sure that we don't send null fields and instead erase fields.

This also works well with deserialization because Option<> parses null, so we can get rid of try_from_json() and ContextError

rasendubi · 2025-02-12T23:19:01Z

eppo_core/src/event_ingestion/event_ingestion.rs

+        let event_delivery_clone = Arc::clone(&event_delivery);
+        let (context_sender, mut context_rx) = mpsc::channel::<(String, ContextValue)>(100);
+        runtime.spawn_untracked(async move {
+            while let Some((key, value)) = context_rx.recv().await {
+                let mut event_delivery = event_delivery_clone.lock().await;
+                event_delivery.attach_context(key, value)
+            }
+        });


minor (not blocking): this mostly works but seems a bit wasteful and may suffer from congestions on event delivery. Given that context is only read/written very briefly, it seems wasteful to lock the whole event delivery with an async mutex (where a sync mutex/rwlock on context would suffice)

rasendubi · 2025-02-12T23:22:58Z

eppo_core/src/event_ingestion/event_ingestion.rs

+        )));
+
+        let event_delivery_clone = Arc::clone(&event_delivery);
+        let (context_sender, mut context_rx) = mpsc::channel::<(String, ContextValue)>(100);


minor: Using a separate channel for context does not guarantee ordering and may suffer from partial writes (which is probably unexpected by users)

Given the following piece of user code:

client.set_context("field1", "hello") client.set_context("field2", "world") client.track(event)

The event might get sent out with only field1 in the context (but not field2), or with no context at all.

rasendubi · 2025-02-12T23:23:35Z

eppo_core/src/lib.rs

 #[cfg(feature = "event_ingestion")]
+pub use event_ingestion::ContextValue;
 pub use sdk_key::SdkKey;


Probably unintended change of API: SdkKey is now available without event_ingestion feature flag

rasendubi · 2025-02-12T23:24:01Z

eppo_core/src/lib.rs

@@ -80,5 +80,6 @@ pub use attributes::{
 pub use configuration::Configuration;
 pub use error::{Error, EvaluationError, Result};
 #[cfg(feature = "event_ingestion")]
+pub use event_ingestion::ContextValue;


Is there any reason you want to expose it at the top level instead of keeping it scoped to event_ingestion mod?

Co-authored-by: Oleksii Shmalko <[email protected]>

felipecsl added 2 commits February 10, 2025 10:45

feat(events): Allow attaching context to event dispatching

9f56940

add ruby api

89be230

felipecsl requested review from rasendubi and petzel February 10, 2025 18:52

rasendubi reviewed Feb 11, 2025

View reviewed changes

replace MutEventIngestion with RefCell

a50e240

felipecsl force-pushed the ff-3932--context branch from b833266 to a50e240 Compare February 11, 2025 17:53

felipecsl added 2 commits February 11, 2025 10:39

fix sharing context between ingestion and delivery

5a4d26e

fmt

3e70d02

felipecsl requested a review from rasendubi February 11, 2025 18:40

patch release

28acc7f

felipecsl commented Feb 11, 2025

View reviewed changes

felipecsl added 4 commits February 11, 2025 10:44

better error handling

04c4c33

switch to context value enum

33ce145

fix build

0fdc926

remove unneeded company

4a6d1ab

rasendubi reviewed Feb 12, 2025

View reviewed changes

Update eppo_core/src/event_ingestion/delivery.rs

1e17dea

Co-authored-by: Oleksii Shmalko <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(events): Allow attaching context to event dispatching #205

feat(events): Allow attaching context to event dispatching #205

felipecsl commented Feb 10, 2025

changeset-bot bot commented Feb 10, 2025 •

edited

Loading

rasendubi left a comment

rasendubi Feb 11, 2025

felipecsl Feb 11, 2025

rasendubi Feb 11, 2025

felipecsl Feb 11, 2025

felipecsl Feb 11, 2025

rasendubi Feb 11, 2025

felipecsl Feb 11, 2025

rasendubi Feb 12, 2025 •

edited

Loading

felipecsl Feb 11, 2025 •

edited

Loading

rasendubi Feb 12, 2025

rasendubi Feb 12, 2025

rasendubi Feb 12, 2025 •

edited

Loading

rasendubi Feb 12, 2025

rasendubi Feb 12, 2025

feat(events): Allow attaching context to event dispatching #205

Are you sure you want to change the base?

feat(events): Allow attaching context to event dispatching #205

Conversation

felipecsl commented Feb 10, 2025

Description

Testing

changeset-bot bot commented Feb 10, 2025 • edited Loading

🦋 Changeset detected

rasendubi 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

rasendubi Feb 12, 2025 • edited Loading

Choose a reason for hiding this comment

felipecsl Feb 11, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rasendubi Feb 12, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

changeset-bot bot commented Feb 10, 2025 •

edited

Loading

rasendubi Feb 12, 2025 •

edited

Loading

felipecsl Feb 11, 2025 •

edited

Loading

rasendubi Feb 12, 2025 •

edited

Loading