autonomys / substation

Polkadot Telemetry service
GNU General Public License v3.0
3 stars 2 forks source link

Send all changes periodically #43

Closed i1i1 closed 1 year ago

i1i1 commented 1 year ago

Fixes #36

This pr updates telemetry core service in such a way that it sends all the updates within some interval. It does that by having 2 copies of node data. It works as follows:

If we receive some data from nodes, we just update the new copy and push telemetry messages to the feed serializer.

Some tests right now are failing, but frontend seem to work fine.

i1i1 commented 1 year ago

I got stuck at "Add batching for node updates within state wrapper". Looks like you moved and refactored a lot of stuff at the same time, so being unfamiliar with the original codebase I don't understand what is going on there.

Yeah, I'm sorry about the commit history. This one should be probably split into several commits. The thing about this commit is that it actually just moves all the serialization from InnerLoop structure into State, so that it would be batched. So you can mirror and see that the actual diff is very small.

I can't say I agree with the PR description either. What I can imagine is the following:

  • we keep up to date state in memory, so for new client we always send latest state, not the old one
  • updates are not sent on interval, it instead sends updates immediately IF update of the same kind wasn't seen within last X seconds and if it was, then we keep updating intermediate state, but only send the update (with whatever latest values are) once X seconds elapse

It looks way harder to implement. Current pr's version doesn't mess up with diffing state, it just writes updates to the serializer and diffs only chain state (like name and node count). So it would be easier to upstream it, I think.

I'd also argue this is what probably needs to be tweakable upstream

Yeah, I agree. The point is that we currently diverged significantly from upstream. After finalizing pr, I would spend some time and send it upstream. WDUT?

i1i1 commented 1 year ago

PR title doesn't make a lot of sense

Yeah, thanks a lot! Updated the title.

I didn't see (maybe I misunderstood something) that you de-duplicate similar update

Yeah, I didn't (just pushed new commits which do though). But in the end grouping everything let to deduplicate updates.

nazar-pc commented 1 year ago

Not sure you noticed, but ever since we deployed this update backend connections to telemetry started dropping again, most likely due to exactly the same reason why we disabled node list in the first place.