FeatureRequest: Better exception logging in case of feed processing errors

[hbruch]

In case a station_status feed (e.g. this one) does not provide vehicle_types_available, a (caught) NPE is reported (see below).

I'm a bit in conflict, if lamassu should handle such issues and work around erroneous feeds to have some cleaner error messages/warnings in the logs. On the other hand, starting to fix and work around might be the road to hell, as there will always be issues which should be fixed at the root and not worked around...

Anyway, exception logs would be much more helpful, if they provided the corrupted feed's system_id.

 {"serviceContext":{"service":"lamassu"},"message":"Caught exception in ForkJoinPool\njava.lang.NullPointerException: null\n\tat java.base/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method)\n\tat java.base/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:77)\n\tat java.base/jdk.internal.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45)\n\tat java.base/java.lang.reflect.Constructor.newInstanceWithCaller(Constructor.java:499)\n\tat java.base/java.lang.reflect.Constructor.newInstance(Constructor.java:480)\n\tat java.base/java.util.concurrent.ForkJoinTask.getThrowableException(ForkJoinTask.java:564)\n\tat java.base/java.util.concurrent.ForkJoinTask.reportException(ForkJoinTask.java:591)\n\tat java.base/java.util.concurrent.ForkJoinTask.invoke(ForkJoinTask.java:689)\n\tat java.base/java.util.stream.ForEachOps$ForEachOp.evaluateParallel(ForEachOps.java:159)\n\tat java.base/java.util.stream.ForEachOps$ForEachOp$OfRef.evaluateParallel(ForEachOps.java:173)\n\tat java.base/java.util.stream.AbstractPipeline.evaluate(AbstractPipeline.java:233)\n\tat java.base/java.util.stream.ReferencePipeline.forEach(ReferencePipeline.java:596)\n\tat java.base/java.util.stream.ReferencePipeline$Head.forEach(ReferencePipeline.java:765)\n\tat org.entur.gbfs.GbfsSubscriptionManager.lambda$update$0(GbfsSubscriptionManager.java:68)\n\tat java.base/java.util.concurrent.ForkJoinTask$RunnableExecuteAction.exec(ForkJoinTask.java:1395)\n\tat java.base/java.util.concurrent.ForkJoinTask.doExec(ForkJoinTask.java:373)\n\tat java.base/java.util.concurrent.ForkJoinPool$WorkQueue.topLevelExec(ForkJoinPool.java:1182)\n\tat java.base/java.util.concurrent.ForkJoinPool.scan(ForkJoinPool.java:1655)\n\tat java.base/java.util.concurrent.ForkJoinPool.runWorker(ForkJoinPool.java:1622)\n\tat java.base/java.util.concurrent.ForkJoinWorkerThread.run(ForkJoinWorkerThread.java:165)\nCaused by: java.lang.NullPointerException: Cannot invoke \"java.util.List.stream()\" because the return value of \"org.entur.lamassu.model.entities.Station.getVehicleTypesAvailable()\" is null\n\tat org.entur.lamassu.util.SpatialIndexIdUtil.createStationSpatialIndexId(SpatialIndexIdUtil.java:43)\n\tat org.entur.lamassu.leader.entityupdater.StationsUpdater.lambda$addOrUpdateStations$4(StationsUpdater.java:180)\n\tat java.base/java.util.HashMap.forEach(HashMap.java:1421)\n\tat org.entur.lamassu.leader.entityupdater.StationsUpdater.addOrUpdateStations(StationsUpdater.java:179)\n\tat org.entur.lamassu.leader.entityupdater.EntityCachesUpdater.updateEntityCaches(EntityCachesUpdater.java:55)\n\tat org.entur.lamassu.leader.FeedUpdater.receiveUpdate(FeedUpdater.java:164)\n\tat org.entur.lamassu.leader.FeedUpdater.lambda$createSubscription$1(FeedUpdater.java:126)\n\tat org.entur.gbfs.GbfsSubscription.update(GbfsSubscription.java:104)\n\tat java.base/java.util.stream.ForEachOps$ForEachOp$OfRef.accept(ForEachOps.java:183)\n\tat java.base/java.util.concurrent.ConcurrentHashMap$ValueSpliterator.forEachRemaining(ConcurrentHashMap.java:3612)\n\tat java.base/java.util.stream.AbstractPipeline.copyInto(AbstractPipeline.java:509)\n\tat java.base/java.util.stream.ForEachOps$ForEachTask.compute(ForEachOps.java:290)\n\tat java.base/java.util.concurrent.CountedCompleter.exec(CountedCompleter.java:754)\n\t... 5 common frames omitted\n","reportLocation":{"filePath":"org.entur.lamassu.leader.FeedUpdater","lineNumber":"95","functionName":"lambda$start$0"},"severity":"WARNING"}

[hbruch]

Besides handling the NPE, it would be very helpful, if lamassu would report, which feed's update failed, e.g. by catching and rethrowing with a wrapping e.g. FeedUpdateException with the feed's system_id, so that one can easily find out, which feed causes issues
.

[testower]

I agree, the current handling is not great https://github.com/entur/lamassu/blob/master/src/main/java/org/entur/lamassu/leader/FeedUpdater.java#L95

Very open to suggestions for improvements here.

[testower]

I think we need to pass a Logger instance to the subscription manager here, and add error handling (try/catch) around calling update on each subscription. That way we can extract useful info to log along with the exception. So this is a feature in gbfs-loader-java

[hbruch]

Another example of misleading exceptions is #452. Having GbfsSubscription.update throw a wrapping custom Exception indicating the system_id would really be helpful and would not require passing a logger.