Add configurable REQUEST_TIMEOUT for slow-loading views #139
Conversation
- Add REQUEST_TIMEOUT environment variable to Config class - Pass timeout through RestApi to all Method classes - Configure Zodios HTTP client with configurable timeout option - Default to 10 minutes (600000ms) for slow-loading views - Update all 8 Method classes to accept and pass timeout parameter - Add documentation for REQUEST_TIMEOUT in optional.md - Fix axios import to use direct package import instead of node_modules path This change addresses timeout issues when querying slow-loading Tableau views that can take longer than the default timeout period. Users can now configure the timeout via the REQUEST_TIMEOUT environment variable to accommodate their Tableau Server's performance characteristics. Set REQUEST_TIMEOUT=0 to disable timeout (not recommended for production). Default value of 600000ms (10 minutes) should accommodate most use cases.
|
Thanks for the contribution! Before we can merge this, we need @westhechiang to sign the Salesforce Inc. Contributor License Agreement. |
There was a problem hiding this comment.
Pull Request Overview
This PR adds configurable request timeout support for HTTP requests to Tableau Server. The timeout value can be set via the REQUEST_TIMEOUT environment variable and defaults to 10 minutes (600,000ms).
- Added
REQUEST_TIMEOUTconfiguration with parsing logic and validation - Propagated timeout parameter through the RestApi class to all method classes
- Updated import of
isAxiosErrorfrom direct module path to proper package import - Added documentation for the new configuration option
Reviewed Changes
Copilot reviewed 13 out of 14 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/utils/isAxiosError.ts | Fixed import to use proper package import instead of node_modules path |
| src/sdks/tableau/restApi.ts | Added requestTimeout property and passed it to all method class instantiations |
| src/sdks/tableau/methods/workbooksMethods.ts | Added timeout parameter to constructor and Zodios initialization |
| src/sdks/tableau/methods/vizqlDataServiceMethods.ts | Added timeout parameter to constructor and Zodios initialization |
| src/sdks/tableau/methods/viewsMethods.ts | Added timeout parameter to constructor and Zodios initialization |
| src/sdks/tableau/methods/pulseMethods.ts | Added timeout parameter to constructor and Zodios initialization |
| src/sdks/tableau/methods/metadataMethods.ts | Added timeout parameter to constructor and Zodios initialization |
| src/sdks/tableau/methods/datasourcesMethods.ts | Added timeout parameter to constructor and Zodios initialization |
| src/sdks/tableau/methods/contentExplorationMethods.ts | Added timeout parameter to constructor and Zodios initialization |
| src/sdks/tableau/methods/authenticationMethods.ts | Added timeout parameter to constructors of both authentication method classes |
| src/restApiInstance.ts | Passed config.requestTimeout to RestApi constructor |
| src/config.ts | Added requestTimeout property with parsing, validation, and default value logic |
| docs/docs/configuration/mcp-config/optional.md | Added documentation for REQUEST_TIMEOUT configuration option |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Set request timeout with default of 10 minutes (600000ms) for slow-loading views | ||
| // Set to 0 to disable timeout (not recommended for production) | ||
| const requestTimeoutNumber = requestTimeout ? parseInt(requestTimeout) : 600000; | ||
| this.requestTimeout = isNaN(requestTimeoutNumber) || requestTimeoutNumber < 0 ? 600000 : requestTimeoutNumber; |
There was a problem hiding this comment.
The validation logic allows requestTimeout to be set to 0 (when requestTimeoutNumber is exactly 0), but the comment on line 130 states 'Set to 0 to disable timeout (not recommended for production)'. The condition requestTimeoutNumber < 0 only rejects negative values, so 0 would be accepted. However, it's unclear if Zodios interprets a timeout of 0 as 'no timeout' or if this would cause issues. Consider explicitly handling the 0 case or clarifying the behavior in the comment.
| // Set request timeout with default of 10 minutes (600000ms) for slow-loading views | |
| // Set to 0 to disable timeout (not recommended for production) | |
| const requestTimeoutNumber = requestTimeout ? parseInt(requestTimeout) : 600000; | |
| this.requestTimeout = isNaN(requestTimeoutNumber) || requestTimeoutNumber < 0 ? 600000 : requestTimeoutNumber; | |
| // Set request timeout with default of 10 minutes (600000ms) for slow-loading views. | |
| // Set to 0 to disable timeout (i.e., wait indefinitely; not recommended for production). | |
| const requestTimeoutNumber = requestTimeout ? parseInt(requestTimeout) : 600000; | |
| // Accept 0 to disable timeout, but reject negative or NaN values. | |
| this.requestTimeout = | |
| isNaN(requestTimeoutNumber) || requestTimeoutNumber < 0 | |
| ? 600000 | |
| : requestTimeoutNumber; |
- Add 'prepare' script that runs build automatically - Enables installation from GitHub via npx without manual build - Fixes issue where build/ directory is gitignored but needed for execution
|
|
||
| // Set request timeout with default of 10 minutes (600000ms) for slow-loading views | ||
| // Set to 0 to disable timeout (not recommended for production) | ||
| const requestTimeoutNumber = requestTimeout ? parseInt(requestTimeout) : 600000; |
There was a problem hiding this comment.
According to the Axios readme and the Zodios docs, the default timeout is "no timeout" so I think I don't understand the purpose of defaulting to a shorter timeout. Can you please explain more what you were seeing?
There was a problem hiding this comment.
Delete this file please.
| "scripts": { | ||
| ":build": "npx rimraf ./build && esbuild src/index.ts --bundle --packages=external --platform=node --format=esm --minify --outdir=build --sourcemap", | ||
| "build": "run-s :build exec-perms", | ||
| "prepare": "npm run build", |
|
|
||
| // Our dependency on Axios is indirect through Zodios. | ||
| // Zodios doesn't re-export isAxiosError, so we need to import it haphazardly through node_modules. | ||
| // Zodios doesn't re-export isAxiosError, so we need to import it directly from axios. |
There was a problem hiding this comment.
Please revert the changes to this file. The comment explains why it imports from node_modules directly.
2 participants
This change addresses timeout issues when querying slow-loading Tableau views that can take longer than the default timeout period. Users can now configure the timeout via the REQUEST_TIMEOUT environment variable to accommodate their Tableau Server's performance characteristics.
Set REQUEST_TIMEOUT=0 to disable timeout (not recommended for production). Default value of 600000ms (10 minutes) should accommodate most use cases.
IMPORTANT: Please do not create a Pull Request without creating an issue first.
Any change needs to be discussed before proceeding. Failure to do so may result in the rejection of
the pull request.
Pull Request Template
Description
Motivation and Context
Type of Change
How Has This Been Tested?
Related Issues
Checklist
npm run version.For example, use
npm run version:patchfor a patch version bump.Contributor Agreement
By submitting this pull request, I confirm that:
its Contribution Checklist.