Skip to content
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

Timestamp on EventEnvelope #28331

Closed
jroper opened this issue Dec 9, 2019 · 5 comments
Closed

Timestamp on EventEnvelope #28331

jroper opened this issue Dec 9, 2019 · 5 comments

Comments

@jroper
Copy link
Contributor

@jroper jroper commented Dec 9, 2019

It would be nice if akka.persistence.query.EventEnvelope offered a (perhaps optional) timestamp. This of course isn't needed for journals that use time based UUIDs for offsets, since the timestamp can be calculated from that, but for other journals, the thing that it offers is a trivial way to collect metrics on how long it takes for messages to be processed by the read side, which is a very useful metric to have, it not only indicates when you're getting behind, it also indicates what impact this is having (ie, how out of date your read sides are).

Furthermore, requiring journals to store a timestamp with each event would offer more interesting queries, and in particular, more interesting non domain specific features that can be added, like "give me the state of this entity at this point in time." That currently can't be implemented if there's no timestamp associated with the events.

@patriknw

This comment has been minimized.

Copy link
Member

@patriknw patriknw commented Dec 10, 2019

We have previously said that if time is interesting it should be part of the application specific events, but I can see how it would be useful for this kind of metrics.

Would it be ok that the timestamp is taken in the journal when writing or does it have to be taken earlier in the PersistentActor, or even be possible to assign by the application?

"give me the state of this entity at this point in time." That currently can't be implemented if there's no timestamp associated with the events

I don't think we should encourage this in persistent actors. We have support for replaying up to a seqNr but it's easy to abuse it and start writing events from that earlier state. Warning in docs: https://doc.akka.io/docs/akka/current/persistence.html#recovery-customization

That is better to do on the read-side, and I think it's possible by adding an application specific timestamp in the events.

@renatocaval

This comment has been minimized.

Copy link
Member

@renatocaval renatocaval commented Dec 10, 2019

We recently discussed something similar. We discussed the idea of having a metadata column that we could add metrics information.

That said, we could add a key/value data structure to the envelope and let the plugin implementation, or instrumentation library, decide what should be added as metadata.

@jroper

This comment has been minimized.

Copy link
Contributor Author

@jroper jroper commented Dec 10, 2019

I don't think we should encourage this in persistent actors. We have support for replaying up to a seqNr but it's easy to abuse it and start writing events from that earlier state. Warning in docs: https://doc.akka.io/docs/akka/current/persistence.html#recovery-customization

I think we need to distinguish application features from observability and administration features. If I use a regular CRUD solution, I have a database client that I can use to perform ad-hoc queries on my data, and this is invaluable when it comes to diagnosing problems and understanding what my system is doing. I can also perform administration on it - let's say a bug in my software allowed my data to end up in a non consistent state, I can fix that with ad hoc queries.

The challenge with event sourcing is that I no longer have this. While I can connect to the database and see the events, they don't tell me what the current state is, and it's pretty hard to just understand the events themselves given that they are serialized in blobs. It's much harder, without additional tooling, to make useful observations and diagnosis' over my data. And, if a bug has corrupted my event log somehow, there are no tools in existence that are practical for helping me to resolve that. If I'm lucky, I might find a way that I can roll forward, but some problems will be more fundamental than that, and due to the nature of event sourcing, if to solve a bug I have to put work arounds in my entities event handlers that for example handle events that shouldn't occur in a particular state specially, I'm stuck with maintaining that code for the life of the system - till long after the people that wrote the code, found the original bug and implemented the fix are gone. Even with the best commented code, the nature of such work arounds will be that they are fragile, since by nature they are working with data that doesn't make sense, and if the original people that wrote it and understand exactly how the data didn't make sense are not around anymore, the result will be an unmaintainable nightmare where no one can evolve the code because they don't know how to without breaking it.

Anyway, that's a bit of a tangent, but my point is that there are features that I agree shouldn't be used in application code, but that doesn't mean we shouldn't offer them, because they are essential for maintenance and observability. Creating a new read-side just so I can diagnose a single bug that I have today and will be fixed tomorrow is not a solution, I need to be able to see and manage my data in an ad-hoc manner.

@patriknw

This comment has been minimized.

Copy link
Member

@patriknw patriknw commented Dec 13, 2019

After some discussions elsewhere we decided to add the timestamp. I can probably do that next week.

@patriknw patriknw added this to Sprint backlog in Akka 2.6.x Dec 13, 2019
@patriknw patriknw self-assigned this Dec 13, 2019
@patriknw patriknw moved this from Sprint backlog to In progress in Akka 2.6.x Dec 16, 2019
patriknw added a commit that referenced this issue Dec 16, 2019
* added to EventEnvolope and therefore include case class stuff
  for binary compatibility
* also added in PersistentRepr, which for example is the serialized format in
  LeveldbJournal
patriknw added a commit that referenced this issue Dec 16, 2019
* added to EventEnvolope and therefore include case class stuff
  for binary compatibility
* also added in PersistentRepr, which for example is the serialized format in
  LeveldbJournal
@helena helena moved this from In progress to Reviewing in Akka 2.6.x Dec 17, 2019
raboof added a commit that referenced this issue Jan 7, 2020
* added to EventEnvolope and therefore include case class stuff
  for binary compatibility
* also added in PersistentRepr, which for example is the serialized format in
  LeveldbJournal
@raboof raboof added this to the 2.5.28 milestone Jan 7, 2020
@raboof raboof self-assigned this Jan 7, 2020
raboof added a commit that referenced this issue Jan 7, 2020
* added to EventEnvolope and therefore include case class stuff
  for binary compatibility
* also added in PersistentRepr, which for example is the serialized format in
  LeveldbJournal
raboof added a commit that referenced this issue Jan 7, 2020
* added to EventEnvolope and therefore include case class stuff
  for binary compatibility
* also added in PersistentRepr, which for example is the serialized format in
  LeveldbJournal
@ignasi35

This comment has been minimized.

Copy link
Member

@ignasi35 ignasi35 commented Jan 10, 2020

After some discussions elsewhere we decided to add the timestamp. I can probably do that next week.

@patriknw didthese discussions also cover the metadata field @renatocaval mentioned in #28331 (comment)?

patriknw added a commit that referenced this issue Jan 13, 2020
Timestamp in EventEnvelope, #28331
@patriknw patriknw closed this Jan 13, 2020
Akka 2.6.x automation moved this from Reviewing to Done Jan 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Akka 2.6.x
  
Done
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants
You can’t perform that action at this time.