feat: Download and report WES run log files #319
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,146 @@ | ||
| /* | ||
| * Additional methods for actually retrieving data pointed to by URLs in WES responses. | ||
| */ | ||
|
|
||
| package wes_client | ||
|
|
||
| import ( | ||
| _context "context" | ||
| _ioutil "io/ioutil" | ||
| _nethttp "net/http" | ||
| _neturl "net/url" | ||
| "fmt" | ||
| "strings" | ||
| ) | ||
|
|
||
| // Linger please | ||
| var ( | ||
| _ _context.Context | ||
| ) | ||
|
|
||
| /* | ||
| GetRunLogData Get data linked to by GetRunLog. | ||
| Returns the content of a URL reverenced in a GetRunLog response, if that URL is also on the WES server. | ||
| * @param ctx _context.Context - for authentication, logging, cancellation, deadlines, tracing, etc. Passed from http.Request or context.Background(). | ||
| * @param runId The run ID used for the GetRunLog call | ||
| * @param dataUrl The URL string from the GetRunLog call | ||
| @return string | ||
| */ | ||
| func (a *WorkflowExecutionServiceApiService) GetRunLogData(ctx _context.Context, runId string, dataUrl string) (string, *_nethttp.Response, error) { | ||
| var ( | ||
| localVarHTTPMethod = _nethttp.MethodGet | ||
| localVarPostBody interface{} | ||
| localVarFormFileName string | ||
| localVarReturnValue string | ||
| ) | ||
|
|
||
| // create path and map variables | ||
| localVarPath := a.client.cfg.BasePath + "/runs/{run_id}" | ||
| localVarPath = strings.Replace(localVarPath, "{"+"run_id"+"}", _neturl.PathEscape(parameterToString(runId, "")), -1) | ||
|
|
||
| // Evaluate dataUrl relative to localVarPath and replace localVarPath | ||
| base, err := _neturl.Parse(localVarPath) | ||
| if err != nil { | ||
| return localVarReturnValue, nil, err | ||
| } | ||
| evaluated, err := base.Parse(dataUrl) | ||
| if err != nil { | ||
| return localVarReturnValue, nil, err | ||
| } | ||
| if (evaluated.Scheme != base.Scheme && !strings.HasPrefix(evaluated.Scheme, "http")) { | ||
| // This doesn't look like something we can fetch | ||
| return localVarReturnValue, nil, fmt.Errorf("WES cannot be used to retrieve %s", dataUrl) | ||
| } | ||
| localVarPath = evaluated.String() | ||
|
|
||
| localVarHeaderParams := make(map[string]string) | ||
| files := make(map[string][]byte) | ||
| localVarQueryParams := _neturl.Values{} | ||
| localVarFormParams := _neturl.Values{} | ||
|
|
||
| // to determine the Content-Type header | ||
| localVarHTTPContentTypes := []string{} | ||
|
|
||
| // set Content-Type header | ||
| localVarHTTPContentType := selectHeaderContentType(localVarHTTPContentTypes) | ||
| if localVarHTTPContentType != "" { | ||
| localVarHeaderParams["Content-Type"] = localVarHTTPContentType | ||
| } | ||
|
|
||
| // to determine the Accept header | ||
| localVarHTTPHeaderAccepts := []string{"text/plain"} | ||
|
|
||
| // set Accept header | ||
| localVarHTTPHeaderAccept := selectHeaderAccept(localVarHTTPHeaderAccepts) | ||
| if localVarHTTPHeaderAccept != "" { | ||
| localVarHeaderParams["Accept"] = localVarHTTPHeaderAccept | ||
| } | ||
| r, err := a.client.prepareRequest(ctx, localVarPath, localVarHTTPMethod, localVarPostBody, localVarHeaderParams, localVarQueryParams, localVarFormParams, localVarFormFileName, files) | ||
| if err != nil { | ||
| return localVarReturnValue, nil, err | ||
| } | ||
|
|
||
| localVarHTTPResponse, err := a.client.callAPI(r) | ||
| if err != nil || localVarHTTPResponse == nil { | ||
| return localVarReturnValue, localVarHTTPResponse, err | ||
| } | ||
|
|
||
| localVarBody, err := _ioutil.ReadAll(localVarHTTPResponse.Body) | ||
|
There was a problem hiding this comment. Are we concerned about using There was a problem hiding this comment. I'm not overly concerned about file size here; even a very large workflow log is probably under a gigabyte. Streaming would of course be better, but I didn't want to invest the time to learn, design and implement a good way to stream it here. It's going to the console, so when it starts to get too large for memory it starts to get too large to really be useful. But it's also not a good idea to let a very large file crash the client. (Really we would want to be able to make HEAD and byte range requests or something, and be able to get just a manageable piece from the end of the log, and to let the user then ask for other parts of the log somehow, but I also didn't want to build out that whole system right now.) Maybe implementing streaming wouldn't actually be more work than it's worth. I could revise the download code to be able to handle errors internally while streaming successful responses, and change the return type to whatever type There was a problem hiding this comment. Adjusting this to return a stream, and passing the stream up to the CLI command implementation in |
||
| localVarHTTPResponse.Body.Close() | ||
| if err != nil { | ||
| return localVarReturnValue, localVarHTTPResponse, err | ||
| } | ||
|
|
||
| if localVarHTTPResponse.StatusCode >= 300 { | ||
| newErr := GenericOpenAPIError{ | ||
| body: localVarBody, | ||
| error: localVarHTTPResponse.Status, | ||
| } | ||
| if localVarHTTPResponse.StatusCode == 401 { | ||
| var v ErrorResponse | ||
| err = a.client.decode(&v, localVarBody, localVarHTTPResponse.Header.Get("Content-Type")) | ||
| if err != nil { | ||
| newErr.error = err.Error() | ||
| return localVarReturnValue, localVarHTTPResponse, newErr | ||
| } | ||
| newErr.model = v | ||
| return localVarReturnValue, localVarHTTPResponse, newErr | ||
| } | ||
| if localVarHTTPResponse.StatusCode == 403 { | ||
| var v ErrorResponse | ||
| err = a.client.decode(&v, localVarBody, localVarHTTPResponse.Header.Get("Content-Type")) | ||
| if err != nil { | ||
| newErr.error = err.Error() | ||
| return localVarReturnValue, localVarHTTPResponse, newErr | ||
| } | ||
| newErr.model = v | ||
| return localVarReturnValue, localVarHTTPResponse, newErr | ||
| } | ||
|
There was a problem hiding this comment. See above. It might be reasonable to expect the endpoint to reply with standard HTTP 403/etc. codes, but can we expect the response payload to be something the WES client would be able to decode? There was a problem hiding this comment. The way Toil implements this is by having log-retrieval endpoints as Toil-specific extensions to the WES server, so when they return errors they will probably be WES-style errors (401, 403, 404, or 500, with a JSON body with If we get something else, we definitely need to still return an error, but I'm not sure what kind of error. Right now it looks like it would be There was a problem hiding this comment. I suppose that's reasonable if that's a common case for these URLs. But yeah, I would suggest that we be prepared to just output the raw response of the server in case the error isn't a WES error. Perhaps just printing that something failed in the normal case and printing the raw response from the server in a debug case? |
||
| if localVarHTTPResponse.StatusCode == 404 { | ||
| var v ErrorResponse | ||
| err = a.client.decode(&v, localVarBody, localVarHTTPResponse.Header.Get("Content-Type")) | ||
| if err != nil { | ||
| newErr.error = err.Error() | ||
| return localVarReturnValue, localVarHTTPResponse, newErr | ||
| } | ||
| newErr.model = v | ||
| return localVarReturnValue, localVarHTTPResponse, newErr | ||
| } | ||
| if localVarHTTPResponse.StatusCode == 500 { | ||
| var v ErrorResponse | ||
| err = a.client.decode(&v, localVarBody, localVarHTTPResponse.Header.Get("Content-Type")) | ||
| if err != nil { | ||
| newErr.error = err.Error() | ||
| return localVarReturnValue, localVarHTTPResponse, newErr | ||
| } | ||
| newErr.model = v | ||
| } | ||
| return localVarReturnValue, localVarHTTPResponse, newErr | ||
| } | ||
|
|
||
| // TODO: Handle odd encodings, unacceptable UTF-8, etc. somehow. | ||
| localVarReturnValue = string(localVarBody) | ||
|
|
||
| return localVarReturnValue, localVarHTTPResponse, nil | ||
| } | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like this is inspired by the OpenAPI-generated
api_workflow_execution_service.go? Could we be a little less verbose if we don't think of this as an extension/part of the WES client?I find WES to be a little under-specified in some places, but I'm no expert and am pretty much going off the OpenAPI spec. If I'm wrong about any of my interpretations of what this URL should be and what should be contained there, let me know.
If we're assuming these are just text files available at either an absolute URL or one relative to the WES endpoint, can we just download that file? Do we need to bother with checking the WES-specified error codes and content types? I'm not sure we have any guarantees that the endpoint hosting the file will respond with the error codes/payloads that the WES client is expecting.
Not something we need to solve right now, but what's prompting me to consider a slightly less WES-endpoint-coupled approach is that most of our existing engines run in Batch, and we currently access their logs via Batch+Cloudwatch. Cloudwatch Logs don't have a standard URL scheme, but I suspect we'll (AGC) eventually want to extend this to allow for loading stdout/stderr from an S3 file or a Cloudwatch Log stream as well as via a URL.
For now, I'm just wondering if we can clean up some of this generated-ish code by just adding some plain-old file loading to
packages/cli/internal/pkg/wes/Client.goThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not so much "inspired by" the OpenAPI-generated code as it is "a complete copy-paste of" the OpenAPI-generated code. I did that for consistency with the rest of the WES API implementation, since retrieving these logs is specified in WES (just in English that the code generator can't read). I also did it because I thought it would work better, faster, than me trying to write something from scratch.
If we don't care about consistency, we could streamline this to look a little more like a human wrote it.
It's not quite as simple as just downloading the file; we definitely need to visit
prepareRequest, which does the signing to get the request through AGC's proxy, assuming the URL we got actually points to the server that gave it to us.I left all the error code detection and error response parsing identical to the WES API request I copied from. If we get an error specified by WES, we'll understand it as specified by WES, and if we get some other error, we won't really understand it but we'll report it anyway. Probably nothing bad would happen if I cut the code, but I took leaving it in as the default.
This should eventually be extended to support S3 or CloudWatch URLs; it could even get AGC away from the
|task names.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I'd prefer human-readable code to code that's consistent with the generated code, personally. Generated code is good in that it's generated, but not good for much else 😛
If these URLs being URLs on the WES server itself is common, then I'm fine with continuing to prepare requests and parse errors in the WES style.