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

increase the timeouts for report API calls to 600 seconds #1412

Merged
merged 4 commits into from
Apr 17, 2024

Conversation

spatten
Copy link
Contributor

@spatten spatten commented Apr 16, 2024

Overview

No Ticket

See https://teamfossa.slack.com/archives/C043EM3L96Z/p1713304985656999

We were having a problem running fossa attribution report --format html ... for a project that was taking a long time to run the report.

The fix is to increase the timeout when we hit the reports API.

Acceptance criteria

We can generate the report without timing out.

Testing plan

make install-dev

Do this in an empty directory with no git repo in it or its parents:

fossa-dev report attribution --format html --project 'projectname from slack message' --revision 'revision from slack message'

Repeat with --format json.

Note that it takes > 30 seconds to run, but it does not take forever.

The previous behavior was that it would make the API call, timeout after 30 seconds, and then try again. I don't know if it ever gave up or if it just kept trying forever.

Risks

The report in question takes a bit over a minute to generate. Increasing the timeout to 5 minutes should be enough, but if you think it should be larger (10 minutes?) I'm totally willing to bump it up.

Metrics

References

https://teamfossa.slack.com/archives/C043EM3L96Z/p1713304985656999

Checklist

  • I added tests for this PR's change (or explained in the PR description why tests don't make sense).
  • If this PR introduced a user-visible change, I added documentation into docs/.
  • If this PR added docs, I added links as appropriate to the user manual's ToC in docs/README.ms and gave consideration to how discoverable or not my documentation is.
  • If this change is externally visible, I updated Changelog.md. If this PR did not mark a release, I added my changes into an # Unreleased section at the top.
  • If I made changes to .fossa.yml or fossa-deps.{json.yml}, I updated docs/references/files/*.schema.json AND I have updated example files used by fossa init command. You may also need to update these if you have added/removed new dependency type (e.g. pip) or analysis target type (e.g. poetry).
  • If I made changes to a subcommand's options, I updated docs/references/subcommands/<subcommand>.md.

@spatten spatten marked this pull request as ready for review April 16, 2024 23:46
@spatten spatten requested a review from a team as a code owner April 16, 2024 23:46
@spatten spatten requested a review from csasarak April 16, 2024 23:46
@spatten spatten changed the title increase the timeouts for report API calls to 600 seconds increase the timeouts for report API calls to 300 seconds Apr 16, 2024
@jssblck
Copy link
Member

jssblck commented Apr 16, 2024

Why even have a default timeout? Users can specify one via --timeout if they want, does it actually benefit us in any way to have timeouts otherwise?

@spatten
Copy link
Contributor Author

spatten commented Apr 16, 2024

Why even have a default timeout? Users can specify one via --timeout if they want, does it actually benefit us in any way to have timeouts otherwise?

The default comes from the Network.HTTP.Req library. Search for "responseTimeout" on this page: https://hackage.haskell.org/package/req-3.13.2/docs/Network-HTTP-Req.html#g:12

@jssblck
Copy link
Member

jssblck commented Apr 16, 2024

In that case, is there a benefit to not setting the timeout to some huge value like 600 or even 3600?

@spatten
Copy link
Contributor Author

spatten commented Apr 16, 2024

In that case, is there a benefit to not setting the timeout to some huge value like 600 or even 3600?

Yeah, I'm open to that. I initially had it at 600 seconds and thought it was maybe too much, but I'm happy to go with 600 seconds.

I think we set it to 600 seconds already for one of the VSI endpoints.

@spatten spatten changed the title increase the timeouts for report API calls to 300 seconds increase the timeouts for report API calls to 600 seconds Apr 16, 2024
@@ -1183,6 +1183,8 @@ getAttributionJson apiOpts ProjectRevision{..} = fossaReq $ do
=: True
<> "dependencyInfoOptions[]"
=: packageDownloadUrl
-- Large reports can take over a minute to generate, so increase the timeout to 10 minutes
<> responseTimeoutSeconds 600
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems fine, but I think it would be good to have a top-level defaultResponseTimeout and refer to that rather than write 600 in both places.

Copy link
Contributor Author

@spatten spatten Apr 17, 2024

Choose a reason for hiding this comment

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

The default is actually 30 seconds (from the library), and we are overriding it here.

The default comes from the Network.HTTP.Req library. Search for "responseTimeout" on this page: https://hackage.haskell.org/package/req-3.13.2/docs/Network-HTTP-Req.html#g:12

@spatten spatten enabled auto-merge (squash) April 17, 2024 17:00
@spatten spatten merged commit 554964d into master Apr 17, 2024
16 checks passed
@spatten spatten deleted the increase-report-request-timeout branch April 17, 2024 17:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants