feat: api to transform events to conclusion events #508

samika98 · 2024-08-29T17:41:13Z

API that processes a series of raw events, transforming them into a structured vector of conclusion events. The transformation involves taking a high watermark as input, fetching events from the database based on this marker, and converting these events into the ConclusionEvent type.

Added transform_raw_events_to_conclusion_events function which converts events based on the provided high watermark
Modified event and payload structures
Updated CeramicEventService to include new transformation logic

TODO : Add time event creation logic for test

samika98 · 2024-08-29T17:46:45Z

service/src/event/service.rs

+                        controller: event.get_controller().unwrap(),
+                        dimensions: vec![], // You may need to populate this if needed
+                    },
+                    // TODO_Discuss: How can this ever have more than one value? prev return a single cid


prev returns a single cid

This is expected, however in the future we may change the Ceramic event format to allow multiple previous values.

samika98 · 2024-08-29T17:47:22Z

service/src/event/service.rs

+                    previous: vec![*time_event.prev()],
+                    //TODO_Discuss: How to determine an ever incrementing index.
+                    //Soln_proposed : We have to store this on disk and increment with every event seen ?
+                    //Define purpose of this counter and how it can be used


** how it can be useful to the consumer of this api

The index needs to come from the db query. I believe its the deliverable column on the data. We need to select that column and preserve it.

nathanielc · 2024-08-29T18:30:54Z

service/src/event/service.rs

+        limit: i64,
+        include_data: ceramic_api::IncludeEventData,
+    ) -> anyhow::Result<Vec<ConclusionEvent>> {
+        let raw_events = ceramic_api::EventStore::events_since_highwater_mark(


We do not want to rely on the API methods here. We should likely move the body of ceramic_api::EventStore::events_since_highwater_mark to a method on the struct directly. Both this method and the EventStore implementation can call it. This way we do not overload the ceramic_api types.

nathanielc · 2024-08-29T18:31:38Z

service/src/event/service.rs

+        &self,
+        highwater_mark: i64,
+        limit: i64,
+        include_data: ceramic_api::IncludeEventData,


We always want the data for the conclusion API so we should not let this be a parameter on the function.

nathanielc · 2024-08-29T18:32:56Z

service/src/event/service.rs

+    // We map over this to get a vec of conclusion events
+    fn transform_raw_events_to_conclusion_events(
+        &self,
+        raw_event: ceramic_api::EventDataResult,


As noted, we should not use the ceramic_api types here but instead use the unvalidated::Event directly.

nathanielc · 2024-08-29T18:33:54Z

service/src/event/service.rs

+        match event {
+            ceramic_event::unvalidated::Event::Time(time_event) => {
+                let mut current_event: Event<Ipld> = event;
+                let mut stream_id = current_event.id();


nit this is stream_cid not the stream id

nathanielc · 2024-08-29T18:43:55Z

service/src/event/service.rs

+                let mut stream_id = current_event.id();
+                let mut init_event: Option<Event<Ipld>> = None;
+                // Traverse the chain of events until we find the init event
+                while let Some(prev_cid) = current_event.prev() {


We do not need to traverse the chain, the event.id() is the CID of the init event already. I imagine this can look something like:

let stream_cid = current_event.id(); let init_event = self.get_event_by_cid(stream_cid)?; let init = ConclusionInit::try_from(init_event)?;// TODO implement this

Constructing a ConclusionInit from an init event means finding the init payload if it exists and gathering its metadata.

We should add an init_payload() method to unvalidated::Event that is similar to is_init method but that returns an Option<&init::Payload>. Then try_from becomes

fn try_from(value: unvalidated::Event) -> Result<ConclusionInit> { let init_payload = value.init_payload().ok_or_else(|| anyhow!("no init payload found")?; Ok(ConclusionInit{ ... }) }

nathanielc · 2024-08-29T18:44:19Z

service/src/event/service.rs

+
+                Ok(ConclusionEvent::Time(ConclusionTime {
+                    event_cid: raw_event.id,
+                    init: ConclusionInit {


Once the above is complete then this just becomes init: init.

nathanielc · 2024-08-29T18:45:05Z

service/src/event/service.rs

+                    previous: vec![*time_event.prev()],
+                    //TODO_Discuss: How to determine an ever incrementing index.
+                    //Soln_proposed : We have to store this on disk and increment with every event seen ?
+                    //Define purpose of this counter and how it can be used


The index needs to come from the db query. I believe its the deliverable column on the data. We need to select that column and preserve it.

samika98 · 2024-09-03T20:27:10Z

service/src/event/service.rs

+                                    e
+                                ))
+                            })?,
+                            index: delivered as u64, // TODO: Implement proper indexing


nit : remove todo

samika98 · 2024-09-03T20:30:41Z

event/src/unvalidated/event.rs

-    fn from(value: init::Payload<D>) -> Self {
-        Self::Unsigned(value)
+// TODO : Delete this? then delete all
+// Delete event with new


@nathanielc All cannot be delete since rest of the two impls are being used in migration code. Makes sense to keep all 3 then?

Yes lets keep them

nathanielc

I like it, a few questions below. The refactor worked out well.

nathanielc · 2024-09-04T14:42:21Z

event/src/unvalidated/event.rs

@@ -92,16 +107,17 @@ where
        }
    }

+    // TODO : Returns the cid no option, update the comment


nit, this todo is complete

nathanielc · 2024-09-04T14:42:51Z

event/src/unvalidated/event.rs

@@ -92,16 +107,17 @@ where
        }
    }

+    // TODO : Returns the cid no option, update the comment
    /// Returns the 'id' field of the event, which is the Cid of the stream's init event.
    /// If this event *is* the init event, then it doesn't know its own Cid and returns None.


Update this comment to indicate we always return the cid because and init event does know its cid

nathanielc · 2024-09-04T14:43:33Z

event/src/unvalidated/event.rs

-    fn from(value: init::Payload<D>) -> Self {
-        Self::Unsigned(value)
+// TODO : Delete this? then delete all
+// Delete event with new


Yes lets keep them

nathanielc · 2024-09-04T14:44:43Z

flight/src/types.rs

@@ -22,11 +25,11 @@ impl ConclusionEvent {
    }
 }

+//TODO : Add Stream Type here


we have an issue to track this, so we can remove this comment for now

nathanielc · 2024-09-04T14:48:48Z

service/src/event/service.rs

+        let stream_cid = raw_event.id();
+        let init_event = self.get_event_by_cid(stream_cid).await?;
+        let init = ConclusionInit::try_from(init_event).unwrap();
+        let event_cid = *raw_event.id();


This is incorrect, the event_cid is not the stream_cid (unless its an init event). Does the raw_event know its own cid? That is what we need to use. Maybe we need a new method on unvalidated::Event to get the cid of itself.

we can also pass it as a parameter into this method

nathanielc · 2024-09-04T14:52:00Z

store/src/sql/access/event.rs

        limit: i64,
-    ) -> Result<(i64, Vec<(Cid, unvalidated::Event<Ipld>)>)> {
+    ) -> Result<(i64, Vec<(Cid, unvalidated::Event<Ipld>, i64)>)> {


Can you change this to use a struct instead of a tuple? Its confusing what the different i64s are for. I get it after reading the code, however a struct would make it more clear.

nathanielc · 2024-09-04T14:53:01Z

store/src/sql/access/event.rs

                .bind(limit)
                .fetch_all(pool.reader())
                .await?;

-        // default to the passed in value if there are no new events to avoid the client going back to 0


why was this comment removed? It still seems valid.

nathanielc · 2024-09-04T14:53:28Z

store/src/sql/access/event.rs

        all_blocks.sort_by(|a, b| a.new_highwater_mark.cmp(&b.new_highwater_mark));
-        let blocks = all_blocks.into_iter().map(|b| b.block).collect();
+
+        let blocks = all_blocks.clone().into_iter().map(|b| b.block).collect();


Why do we have to clone the blocks?

nathanielc · 2024-09-04T15:52:26Z

I'd like to see a test that shows we are correctly converting from events to conclusion events

samika98 · 2024-09-05T19:04:23Z

store/src/sql/access/event.rs

-        Ok((max_highwater, values))
+        let result = values
+            .into_iter()
+            .zip(all_blocks.iter())


we use them here as well, we add the delivered field to all the blocks here

dav1do · 2024-09-06T02:42:30Z

service/src/event/ordering_task.rs

-                .id()
-                .expect("prev must exist for non-init events");
+            let stream_cid = parsed_event.id();
+            let prev = parsed_event.id();


This should be prev().. it looks like we introduced a bug here that was released in v0.33 (made latest this week)... fortunately it's the startup task code and has probably never done anything. We don't have long streams currently so things shouldn't be getting picked up at startup, meaning we we're probably okay without doing any clean up, but I'm going to add a ticket to investigate how it got through/improve the tests.

fixed in #516

… discussion

…vered in creating conclusion event

…termark

samika98 commented Aug 29, 2024

View reviewed changes

nathanielc reviewed Aug 29, 2024

View reviewed changes

samika98 force-pushed the feat/convert-raw-events-to-conclusion-type branch from 599eaab to c04e093 Compare September 3, 2024 19:33

samika98 changed the title ~~feat: add basic structure to lead discussion + add multiple todos for…~~ feat: api to transform events to conclusion events Sep 3, 2024

samika98 temporarily deployed to github-tests-2024 September 3, 2024 19:44 — with GitHub Actions Inactive

samika98 marked this pull request as ready for review September 3, 2024 20:25

samika98 requested review from dav1do and a team as code owners September 3, 2024 20:25

samika98 requested review from smrz2001 and nathanielc and removed request for a team September 3, 2024 20:25

samika98 commented Sep 3, 2024

View reviewed changes

nathanielc requested changes Sep 4, 2024

View reviewed changes

samika98 temporarily deployed to github-tests-2024 September 5, 2024 17:38 — with GitHub Actions Inactive

samika98 commented Sep 5, 2024

View reviewed changes

samika98 temporarily deployed to github-tests-2024 September 5, 2024 19:15 — with GitHub Actions Inactive

dav1do reviewed Sep 6, 2024

View reviewed changes

Samika Kashyap added 7 commits September 9, 2024 10:20

feat: add basic structure to lead discussion + add multiple todos for…

6209bcd

… discussion

feat: make events_since_highwater return a delivered field + use deli…

5fa2817

…vered in creating conclusion event

fix: review comments

5c63680

chore: cleanup

012df7f

chore: minor refactor to simplify logic to fetch events since high wa…

62100c1

…termark

feat: test that fetches conclusion events since a highwater mark

dd58582

chore: rebase

97dd107

samika98 force-pushed the feat/convert-raw-events-to-conclusion-type branch from efb7972 to 97dd107 Compare September 9, 2024 17:46

samika98 temporarily deployed to github-tests-2024 September 9, 2024 17:57 — with GitHub Actions Inactive

feat: test

b0a36ae

samika98 requested a review from nathanielc September 9, 2024 21:12

samika98 temporarily deployed to github-tests-2024 September 9, 2024 21:23 — with GitHub Actions Inactive

nathanielc approved these changes Sep 10, 2024

View reviewed changes

samika98 added this pull request to the merge queue Sep 10, 2024

Merged via the queue into main with commit 0af708d Sep 10, 2024
6 checks passed

samika98 deleted the feat/convert-raw-events-to-conclusion-type branch September 10, 2024 15:41

smrz2001 mentioned this pull request Sep 18, 2024

chore: version v0.36.0 #533

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: api to transform events to conclusion events #508

feat: api to transform events to conclusion events #508

samika98 commented Aug 29, 2024 •

edited

Loading

samika98 Aug 29, 2024

nathanielc Aug 29, 2024

samika98 Aug 29, 2024

nathanielc Aug 29, 2024

nathanielc Aug 29, 2024

nathanielc Aug 29, 2024

nathanielc Aug 29, 2024

nathanielc Aug 29, 2024

nathanielc Aug 29, 2024

nathanielc Aug 29, 2024

nathanielc Aug 29, 2024

samika98 Sep 3, 2024

samika98 Sep 3, 2024

nathanielc Sep 4, 2024

nathanielc left a comment •

edited

Loading

nathanielc Sep 4, 2024

nathanielc Sep 4, 2024

nathanielc Sep 4, 2024

nathanielc Sep 4, 2024

nathanielc Sep 4, 2024

samika98 Sep 5, 2024

nathanielc Sep 4, 2024

nathanielc Sep 4, 2024

nathanielc Sep 4, 2024

nathanielc commented Sep 4, 2024

samika98 Sep 5, 2024

dav1do Sep 6, 2024

dav1do Sep 6, 2024

feat: api to transform events to conclusion events #508

feat: api to transform events to conclusion events #508

Conversation

samika98 commented Aug 29, 2024 • edited Loading

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

Choose a reason for hiding this comment

nathanielc left a comment • edited Loading

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

nathanielc commented Sep 4, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samika98 commented Aug 29, 2024 •

edited

Loading

nathanielc left a comment •

edited

Loading