unleash-client-rust icon indicating copy to clipboard operation
unleash-client-rust copied to clipboard

Feat: Preserve original error message in warn! logs

Open skairunner opened this issue 2 years ago • 3 comments

This PR rearranges the error handling in poll_for_updates. It preserves the original error to aid in debugging.

About the changes

Closes #35 .

Important files

Only one file was touched.

Discussion points

The original code collected whether an error happened in a var that is later checked and logged. I instead embedded the warn! calls right where errors happen. I believe this aids in clarity and also means there can be slightly different error messages depending on the situation, but I am open to other ways of handling it as well.

skairunner avatar May 31 '22 10:05 skairunner

I've tried a different approach, lifting all the update logic into its own fn and returning whenever an error is encountered. It's certainly much less nested, but I'm not sure if it addresses the "key ergonomics around error handling" issue. I would appreciate some feedback!

With regards to opentelemetry, I'm hesitant to implement what sounds like will be a fairly large change. My intention was to do a small, in-place (which i suppose is no longer applicable), uncontroversial fix that would immediately improve my use-case. While I think it would be ideal to integrate observability, unleash-client-rust is already logging here. I don't feel like it would hurt to make those existing logs slightly better.

skairunner avatar Jun 01 '22 09:06 skairunner

I have pushed a change that makes non-JSON responses from the features endpoint be more obvious, a la #37

skairunner avatar Jun 20 '22 12:06 skairunner

Thank you. The tokio patch is merged now, and I'll try to find time to review this in detail later this week or next.

rbtcollins avatar Jun 23 '22 14:06 rbtcollins