ropensci / qualtRics

Download ⬇️ Qualtrics survey data directly into R!
https://docs.ropensci.org/qualtRics
Other
215 stars 70 forks source link

Improve the console messaging when there is an API error, now that httr::RETRY is being use #253

Closed chrisumphlett closed 2 years ago

chrisumphlett commented 2 years ago

That default message from httr::RETRY is vague. If running a call that takes a long time (like list_distribution_links() on a large distribution) there may be errors on multiple calls, resulting in something that looks like:

image

It's not readily apparent if this is one issue, and it's retrying it many times, or multiple issues with a single retry. There could be improved messaging to the console to help the user understand what's happening.

jmobrien commented 2 years ago

@chrisumphlett would it give you what you need if there was a message line showing the survey ID of each request to delineate the groups?

chrisumphlett commented 2 years ago

That's not what I had in mind. Many requests will have a single survey id. Really they all do as far as the qualtRics function. I personally submit use the fetch_survey function on many surveys at once, and call it within a loop where I do print the survey id. But -- the error occurs not on a survey level, but on a page/request level. My example above was calling list_distribution_links on a particular distribution used for a single survey.

Knowing the page is not necessarily that useful. I'm not proposing that the message return specific information. Rather, that it be more descriptive of what is happening so that the user isn't confused (they know that it is retrying the error on purpose, because we believe it was a transient api error/refusal).

jmobrien commented 2 years ago

Yeah, that makes sense, and I'm with you. Do you think these error messages would be beneficial at all, then? We already have a system for reporting any situation where there's an error that wasn't overcome--i.e., the ones that anyone is likely to care about.

chrisumphlett commented 2 years ago

I agree that using the package's standard method is a good idea, however, it gets short-circuited by httr::RETRY. 504 errors are handled in utils.R:

`504` =
          c("Qualtrics API reported a gateway timeout error (504):",
            "These errors are usually resolved by retrying the request",
            glue::glue("instanceId: {httr::content(res)$meta$error$instanceId}"),
            glue::glue("errorCode: {httr::content(res)$meta$error$errorCode}")),

But when I get a 504 error, this is what shows, which is what I think will be confusing: image

jmobrien commented 2 years ago

Right. Looks like RETRY still returns the final value after the max number of attempts (currently 4). So we'd still get the custom error reporting if we were repeatedly getting the error, which is probably all we care about right? I made a minor PR #269 that might address this, basically just setting quiet = TRUE in httr::RETRY() so it only reports an ultimate request failure.

chrisumphlett commented 2 years ago

I didn't know about that option, I think that's fine. Typically it's re-trying quickly, and you're getting a transient error because you're making a request that's taking awhile so I don't think the user is going to notice such a delay that they'd need some kind of message.