Repository navigation
Log the warnings of every cloud response, and a missing product as a warning - #123
Merged
Merged
Conversation
…t a missing product is a warning
…ng product as a warning
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.
Problem
warningsarray beside its results, and no code infiftyone.pipeline.cloudrequestenginereads it. The request engine parses the discovery response for its products (cloudRequestEngine.jslines 404 to 415 at 6d5d4fef) and keeps the processing response as it is (lines 504 to 515), so a warning from the service reaches nobody.cloudEngine.jslines 131 to 141), though nothing is wrong with the request and the other products in the answer are used.What the specification says
From
pipeline-specification/pipeline-elements/cloud-request-engine.mdlines 440 and 441 at 706de1f0:From lines 460 and 461 of the same page, one of those steps:
From
pipeline-specification/pipeline-elements/cloud-aspect-engine.mdlines 63 to 69 at 706de1f0:.NET logs the response's warnings (
CloudRequestEngine.cslines 798 to 807 at f0ba9653), masking the licence key in each as in the errors.What changed
CloudRequestEngine.logWarningslogs each entry of a response'swarningsarray atwarnlevel, with the licence key masked as it is in the errors, because the service's text can name the key it was sent. The discovery response's warnings are logged to every Pipeline the engine is in, and a processing response's to the Pipeline of that request. The response is otherwise used as before, and a response that is not JSON or has nowarningsarray logs nothing.warnlevel, still once for each engine and naming its data key. A null product, which is an answer to that request, is still an error each time.types/cloudRequestEngine.d.tsdeclareslogWarnings, written in the formnpm run tscwrites, and a comment inpipeline.jsno longer says a missing element is logged as an error.Before you upgrade
An application that watches its log for errors no longer gets one when the resource key includes no product that a cloud engine in its Pipeline reads. That is now logged at warning level, once for each engine. Warnings the cloud service sends with a response are now logged at warning level, where they were not logged at all.
Tests
pipeline/fiftyone.pipeline.cloudrequestengine/tests/cloudWarnings.test.jschecks that each warning in a processing response and in the discovery response is logged as a warning with no error, that a licence key a warning names is masked, and that a response with no warnings logs none.pipeline/fiftyone.pipeline.cloudrequestengine/tests/cloudEngineMissingElement.test.js, the missing product is now read from the warnings, once over two requests, naming the data key, with no error, under a resource key and under a licence key alone.Red, run 43622252184 on d939f5b, the test commit on
rc's code, with only a throwaway workflow added. On Ubuntu and on Windows 5 tests fail and the other 620 pass. Three are the new warning tests, "each warning in a response to a request is logged as a warning", "each warning in the discovery response is logged as a warning" and "a license key a warning names is masked", and two are the missing product tests that now look for a warning, "an answer without the element resolves and warns the key lacks it" and "a missing element names requestedProperties as a cause" under a licence key alone.Green, run 43622265112 on 900fead, this head, with only the same throwaway workflow added. All 625 pass on Ubuntu and on Windows.
Types, run 43622274931 ran
npm run tscon this head merged with the head of #113, which writes the types again, and found no difference in the committed types butpipeline/fiftyone.pipeline.did/types/owid-js/v1.d.ts, which tsc writes for the OWID submodule already onrcand which #113 leaves to a later change. So the declaration oflogWarningswritten here by hand is what tsc writes.Full build
Run 43637563411 on
tmp/pr-matrix-123, started bytools/pr-matrix.pywithout macOS. All 7Setting 'PullRequestSha'lines give this head, 900fead, and Configure printed "Left out 2 macOS entries". Each of the six Build and Test jobs, Ubuntu, Ubuntu ARM and Windows on Node.js 22 and 24, passed 1,497 unit tests with 2 skipped and 96 integration tests with 4 skipped.macOS was left out of this draft's full build under the 9 November decision. The rc head is built with macOS before it merges into main.
What remains
Nothing for these two requirements.