Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Fail fast on missing event type #77

Closed
thomaseyde opened this issue Apr 4, 2022 · 12 comments
Closed

Fail fast on missing event type #77

thomaseyde opened this issue Apr 4, 2022 · 12 comments

Comments

@thomaseyde
Copy link

This is related to #4

Eventuous silently stop working when an event type is not registered, which makes it very hard to figure out why things aren't working.

Today I discovered we have obsolete events in our database, and we have managed to do the incredible stupid thing which is to delete or rename those events in our code base. This took us a few hours of debugging and added logging, and even then the discovery was by chance: We happened to log the result of ApplicationService.Handle() which we then could see contained an error message.

It would have been so much easier and faster if an exception was thrown instead.

Faster feedback for new developers would guide us in the right direction faster. Swallowed exceptions have a too high risk of leaving the system in an invalid state.

@bartelink
Copy link
Contributor

bartelink commented Apr 4, 2022

As a counterpoint from a lurker/chicken, in Equinox the default behaviour (when building state) is to not throw or log about this kind of a situation - the thinking being that there are many common cases where you want to ignore some event types:

  • blue/green deploys: you typically don't want the old logic to completely explode because you suddenly have a new event in your store
  • if you once produced some events in a staging environment and are not worried about the consequence, you must keep your dead event type in the mapping until the end of time
  • in any system, over time you inevitably end up with multiple events that are no longer used. There definitely is a period where having this information be in your face in the code is really welcome. But in most long lived systems, there are definitely cases where you don't want to talk about them any more.
  • where you have >1 fold/aggregation on a give stream, you don't want to be having to keep two sets of code in sync (not saying you should, but it does happen)

Obviously if you introduce a new financial instrument in your system and it does not consider the new Withdrawal event to be of consequence, there is absolutely a risk. But it must be balanced with the negatives of having your system be brittle and/or defaulting to mustUnderstand semantics. Having a mustUnderstandAllEventTypes that can be set to True on one's Codec would be useful in that case I guess.

For projections scenarios, I can't imagine anyone wanting logging or exceptions as you're typically white-listing a subset of events

I'd be interested to hear your thoughts as to whether that makes you reconsider or double down...

Would a hook that lets you log or throw in the case of an unknown event type address your desire?

@alexeyzimarev
Copy link
Contributor

Eventuous silently stop working when an event type is not registered, which makes it very hard to figure out why things aren't working.

"Stops working" means what exactly? Subscriptions ignore unknown event types by default

I made quite a few improvements for detecting unregistered event types upfront. Eventuous will throw and fail on startup in the following scenarios:

  • When using On<T> registrations in AggregateState instead of pattern matching
  • Same for projections
  • Same when using EventHandler

@alexeyzimarev
Copy link
Contributor

I might be wrong though. I think I started it, but it might not be 100% done. But, you get the idea.

When using pattern matching in When it's almost impossible figure out if all the event types are registered.

@alexeyzimarev
Copy link
Contributor

alexeyzimarev commented Apr 4, 2022

If you can explain what exactly fails and should throw (and doesn't) - I can look closer.

Then again, if the database has events that aren't available in the application - it will always be failing at runtime.

@alexeyzimarev
Copy link
Contributor

We happened to log the result of ApplicationService.Handle() which we then could see contained an error message.

The app service results Result for a reason, so it normally won't be used as a fire-and-forget. Although I see some complications in integration scenarios when you literally don't have any other option than to throw and stop (human intervention required).

It might be mitigated quite simply by checking the result explicitly with if, by adding an extension to the app service (ThrowOnError) or by adding an option (requires more obtrusive code change).

@thomaseyde
Copy link
Author

It is my code which stops working. In this particular case I was reading and projecting events, and noticed the projector stopped long before reaching the end. This would probably be a less of a problem if we started out with Eventuous, but we didn't. So we have a mix of old projectors in there.

Investigating further, I noticed that in this case it is the ApplicationService.Handle() which fails silently. One case was a missing event type, another was an exception thrown by a library which doesn't support record because of no parameterless ctr.

My problem with this is because of no exception, we got no notification of anything wrong.

I decided to have a look at the source code of ApplicationService while writing this, and I see now that all exceptions are in fact 'swallowed' and put into an ErrorResult.

I see problems with this:

  1. Exceptions are swallowed
  2. It's not clear from the base type that this is exceptional
  3. Nothing encourages or forces us to inspect the return value and take proper action

An empty Result type with a Match(ok => {}, error => {}) would have been nice, which throws when Result is ErrorResult and Match() was never called.

@alexeyzimarev
Copy link
Contributor

Try this from the latest alpha:

    public static IServiceCollection AddApplicationService<T, TAggregate, TState, TId>(
        this IServiceCollection services,
        bool                    throwOnError = false
    )

@alexeyzimarev
Copy link
Contributor

Nothing encourages or forces us to inspect the return value and take proper action

I can't force you to do that, but it's documented https://eventuous.dev/docs/application/app-service/#result

Also, if you add tracing, you will see it right away.

@thomaseyde
Copy link
Author

Try this from the latest alpha:

    public static IServiceCollection AddApplicationService<T, TAggregate, TState, TId>(
        this IServiceCollection services,
        bool                    throwOnError = false
    )

Is it possible without the dependency container?

@alexeyzimarev
Copy link
Contributor

Yes, just look at the code of that extension. You need to wrap your app service to a ThrowingApplicationService.

@alexeyzimarev
Copy link
Contributor

@thomaseyde any feedback?

@thomaseyde
Copy link
Author

I will look into it. I see that the case isn't about not throwing, but that I didn't understand how to catch the error. Now that I do, I can do something about it.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants