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

Gracefully handle errors in subscriptions. #4473

Closed

Conversation

toddburnside
Copy link
Contributor

If this seems like a good approach we could apply it to other subscriptions as well. Maybe pop a toast if there is no data so the user knows they might be out of date and could reload?

For subscriptions where we regularly expect data + errors, like observations, we could only log in the case of no data. That would clean up the console. Maybe we want to only log for no data for all subscriptions? If there is data, we'll assume we're OK?

I don't expect this situation to happen often (no data). If the ODB is sending a subscription event, it seems like there should be data. But, it was happening for awhile until a bug got fixed in the ODB. Just suddenly going to a big screen of spinning planets is not very user friendly behavior. 😄

@mergify mergify bot added the explore label Jan 9, 2025
Copy link

bundlemon bot commented Jan 9, 2025

BundleMon

Files updated (1)
Status Path Size Limits
index-(hash).js
1.73MB (-74B 0%) -
Unchanged files (7)
Status Path Size Limits
exploreworkers-(hash).js
597.76KB -
index-(hash).css
66.01KB -
workbox-window.prod.es5-(hash).js
2.07KB -
plotworker-(hash).js
84B -
agsworker-(hash).js
82B -
catalogworker-(hash).js
82B -
itcworker-(hash).js
81B -

Total files change -74B 0%

Final result: ✅

View report in BundleMon website ➡️


Current branch size history | Target branch size history

@@ -354,12 +365,16 @@ object ProgramCacheController
val updateConfigurationRequests
: Resource[IO, Stream[IO, ProgramSummaries => ProgramSummaries]] =
ProgramQueriesGQL.ConfigurationRequestSubscription
.subscribe[IO](ConfigurationRequestEditInput(props.programId.assign))
.subscribe[IO](ConfigurationRequestEditInput(props.programId.assign))(using
summon,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can the summon be omitted?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll try again, but I don't think so. I think you need something in every position of the using parameter list. The existing subscriptions specifying a error policy are doing this. But, I guess I could maybe just do a given prior to the subscription,

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was under the impression that Scala 3 was now smart enough to type-match givens in case some were not declared. If that's not the case, just leave the summon.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought so true, so I tried that earlier. It may supposed to be that smart, but apparently it is not.

val msg = errors.map(_.message).toList.mkString(": ")
Logger[IO].error(s"Error in subscription: $msg")

def logAndIgnoreErrors[D]: Pipe[IO, GraphQLResponse[D], D] =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice

Copy link
Contributor

@rpiaggio rpiaggio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

I agree to make this pattern more widespread.

Maybe we can use a method or extension to encapsulate the behavior of passing ReturnAlways (or just using that as given) and passing the stream through the logging pipe?

@toddburnside
Copy link
Contributor Author

Maybe we can use a method or extension to encapsulate the behavior of passing ReturnAlways (or just using that as given) and passing the stream through the logging pipe?

I'll look and see if I can come up with something more ergonomic.

@toddburnside
Copy link
Contributor Author

Maybe we can use a method or extension to encapsulate the behavior of passing ReturnAlways (or just using that as given) and passing the stream through the logging pipe?

I wasn't able to find a nice solution to this. The main issue was that the subscribe methods are concrete methods on the GraphQLOperation objects that take as parameters the parameters of the specific Variable case classes. As an alternative, I made a PR in clue that provides this functionality. gemini-hlsw/clue#654

I will push another commit that shows how that could be used in explore. It, of course, won't compile at this time.

@toddburnside toddburnside force-pushed the sc-4509-gracefully-handle-error-in-subscriptions branch from a39afb4 to b5a0f00 Compare January 10, 2025 21:09
@toddburnside
Copy link
Contributor Author

Closed in favor of #4477

@toddburnside toddburnside deleted the sc-4509-gracefully-handle-error-in-subscriptions branch January 16, 2025 22:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants