Pulse discover #167
Pulse discover #167
Conversation
src/tools/pulse/generateInsightBrief/generatePulseInsightBriefTool.test.ts
Outdated
Show resolved
Hide resolved
| mocks.mockGetConfig.mockReturnValue({ | ||
| disableMetadataApiRequests: false, | ||
| boundedContext: { | ||
| projectIds: null, | ||
| datasourceIds: null, | ||
| workbookIds: null, | ||
| }, | ||
| }); |
There was a problem hiding this comment.
Same, I feel like having the mock config may be unnecessary
There was a problem hiding this comment.
keeping this cuz it seems i need it for the scoping
There was a problem hiding this comment.
With tool scoping you will likely need to add back resetResourceAccessCheckerSingleton() in case you ever added a 2nd test that tested scoping.
There was a problem hiding this comment.
hmm tests still pass without it, tho its not clear for me what this does
There was a problem hiding this comment.
The resource access checker singleton caches the results of whether a datasource is allowed or not. Resetting it clears the cache
src/tools/pulse/generateInsightBrief/generatePulseInsightBriefTool.ts
Outdated
Show resolved
Hide resolved
| }); | ||
| }; | ||
|
|
||
| generatePulseInsightBrief = async ( |
There was a problem hiding this comment.
nit: please document this method similar to the other ones in this file
| if (message.metric_group_context) { | ||
| message.metric_group_context = message.metric_group_context.filter( | ||
| (metricContext) => | ||
| datasourceIds.has(metricContext.metric.definition.datasource.id), |
There was a problem hiding this comment.
Non-blocking, but I noticed that Pulse tool scoping checks the datasourceIds from the boundedContext directly, instead of using resourceAccessChecker.isDatasourceAllowed() which also checks if the datasource exists in an allowed project if a project filter is defined. I don't think this was intentional.
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
Today, the tableau mcp server mostly provides direct access to existing APIs on the tableau platform (e.g /search, /query-datasource, /get-workbooks, etc.). However, in the case of the Pulse discover experience, we have invested a lot of time and resources into building an intelligent experience. The goal of integrating the Pulse /discover experience into the Tableau MCP server is to:
Externalize the intelligence that we have implemented natively on the tableau platform
Provide a clearer value proposition for how the MCP server improves as customers leverage the capabilities on our premium SKUs
The intelligence in the discover service is that it has been instructed how to compare and analyze a group of metric insights that belong to a metric group. In addition, it provides relevant follow-up questions based on additional deterministic insights that can be gleaned from the metric group in scope.
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.