Make the ClearlyDefined batch size configurable via -lbs - #1532
Open
ChristophHornung wants to merge 1 commit into
Open
Make the ClearlyDefined batch size configurable via -lbs#1532ChristophHornung wants to merge 1 commit into
ChristophHornung wants to merge 1 commit into
Conversation
Fetching license information sends components to the ClearlyDefined batch API in slices of a hardcoded batch size of 500. Large batches regularly exceed the origin timeout in front of api.clearlydefined.io, which returns HTTP 524; because there is no retry and no per-batch fallback, a single failed response leaves the SBOM with no license information at all. Add a LicenseInformationBatchSize (-lbs) CLI argument so the batch size can be tuned, mirroring the existing LicenseInformationTimeoutInSeconds (-lto) argument added in microsoft#773. The default of 500 preserves the previous behaviour, so this is opt-in only. Addresses microsoft#944. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
Author
|
@microsoft-github-policy-service agree |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Make the ClearlyDefined batch size configurable via
-lbsProblem
When
-li trueis set,LicenseInformationService.FetchLicenseInformationFromAPIsends components to the ClearlyDefined batch API in slices of a hardcodedbatchSize = 500. For a project with a few hundred components that is a single POST tohttps://api.clearlydefined.io/definitions?expand=-filesasking the service to resolve every coordinate at once.That request regularly exceeds the origin timeout in front of
api.clearlydefined.io, which responds HTTP 524. Because there is no retry and no per-batch fallback, one failed response means the SBOM ends up with no license information at all:Raising
-ltodoes not help — the client timeout was already honored here (the request ran a full 125 s); the failure is on the server side, so a longer client timeout just waits longer for a response that never arrives. Splitting the work into smaller requests does help, but the batch size is not reachable from the outside.What this changes
Per the second maintainer takeaway in #944 (expose the batch size as a CLI argument):
LicenseInformationBatchSize(-lbs) generation argument, plumbed throughConfigurationMapper,IConfiguration/Configuration/InputConfiguration, and sanitized inConfigSanitizerexactly like-lto: null → default, non-positive → default with a warning, above max → truncated with a warning, and a warning when-lbsis supplied without-li.LicenseInformationServicetakes the batch size as a parameter instead of hardcoding it.Constants.DefaultLicenseFetchBatchSize = 500,Constants.MaxLicenseFetchBatchSize = 1000.The default of 500 is deliberately the previous hardcoded value, so behaviour is unchanged unless
-lbsis passed. The #944 discussion also floated lowering the default to 100; that is a one-line change on top of this if you would prefer users to get smaller batches without opting in. I kept it opt-in so this PR carries no behavioural change of its own — happy to switch if you would rather have the new default.Notes for reviewers
This touches version-pinned interfaces.
ILicenseInformationService.FetchLicenseInformationFromAPIandILicenseInformationFetcher.FetchLicenseInformationAsynceach gain anint batchSizeparameter, andIConfigurationgains a property;VersionSpecificPins/Version_4_0/InterfaceConcretionTests.csis updated accordingly. The precedent I followed is #1110, which added a property to the pinnedIConfigurationand updated the same pin class, shipping in v4.1.0.If you would rather not change that surface at all, the alternative is to inject
IConfigurationintoLicenseInformationServiceand read the batch size there, leaving both executor interfaces untouched. Happy to switch — just say which you prefer.Not included here: the third takeaway in #944 (a CLI argument to fail the run when license fetching fails). That is independent of this change and also covered by #733; glad to open a separate PR for it.
Two adjacent gaps noticed while doing this — both pre-existing and left alone to keep the diff focused:
src/Microsoft.Sbom.Targets/README.mddocuments<SbomGenerationLicenseInformationTimeoutInSeconds>, but no such property exists inGenerateSbom.cs,Microsoft.Sbom.Targets.targets, orSbomCLIToolTask.cs— the MSBuild task cannot actually set-lto.-lbsis therefore not added to that README either, so as not to document a second option that does nothing.ConfigFile.cshas noLicenseInformationTimeoutInSeconds, so-ltocannot be set through a config file.-lbsmirrors that and is CLI-only.Let me know if you want either folded into this PR.
Testing
LicenseInformationBatchSizesanitizer tests mirroring the existing-ltocoverage: valid values pass through unchanged (1, 500, 1000), out-of-range values are corrected (negative, zero, max+1,int.MaxValue), and null falls back to the default withSettingSource.Default.Addresses #944.