fix(tools): correct regional dimension names and clarify metric vs dimension - #108
Closed
josep-reyero wants to merge 2 commits into
Closed
fix(tools): correct regional dimension names and clarify metric vs dimension#108josep-reyero wants to merge 2 commits into
josep-reyero wants to merge 2 commits into
Conversation
…s dimension Verified against the dbt Semantic Layer API directly: searching list_metrics for "country" or "region" returns zero metrics, because those are dimensions. That is what stranded a real client session — it searched metrics for a dimension concept, got an empty array with no explanation, and abandoned the tool. Also corrects a name this guidance had wrong: the organization HQ dimension is entity-prefixed (activity_project_id__organization_lf_region), not a bare organization_lf_region, so the previous wording would have produced an invalid dimension. Spell out what metrics and dimensions each are, and that list_metrics searches metric names and descriptions only. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Josep Garcia-Reyero Sais <josepreyero@gmail.com>
josep-reyero
added
the
ai-assisted
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review for a one-time review, or @claude review always to subscribe this PR to a review on every future push.
Tip: disable this comment in your organization's Code Review settings.
Contributor
There was a problem hiding this comment.
Pull request overview
Clarifies Semantic Layer metric/dimension discovery and corrects regional dimension guidance.
Changes:
- Explains that metric search excludes dimension names.
- Corrects the organization-region qualified-name example.
- Updates description assertions.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
internal/tools/lens.go |
Improves discovery guidance and regional dimension naming. |
internal/tools/lens_test.go |
Updates description-content assertions. |
The description was 3,662 characters. Descriptions are cut at 2,048 before the model sees them, so 44% of it was invisible — including the entity-prefix rule, the country/region routing tip and the tlf membership caveat. The visible half told the model memberships were an exception without ever saying what to do instead, which is why regional questions kept landing on query_lfx_lens. Rewriting it meant first establishing what the tool actually offers. Probing the live Semantic Layer showed the old framing understated it badly: it is not "pre-aggregated metrics" (MetricFlow compiles SQL per request), and it does not only return numbers — grouping by a name dimension returns a ranked list of named organizations or people, which is exactly the shape of the stakeholder question this work exists to answer. So the description now names the six domains it covers, states that country/region questions always belong here, and describes ranking, trending, multi-dimension breakdown and cross-domain combination. Syntax moved onto the parameter it governs — where carries the MetricFlow forms, group_by the entity__field rule and grain suffixes — because each jsonschema description is a separate field with its own budget, and the model reads it at the moment it fills that field. That redistribution is what made room; the description had been rationing characters against a limit the parameters do not share. describe is renamed help, since it is now a fallback for a failed query rather than a prerequisite. The old name still dispatches so a cached schema does not break. Verified end to end against the live stack rather than by compiling SQL alone: both motivating questions return real data, and the ranked-list example was wrong — it filtered lf_region on 'Asia', which compiles but matches nothing. The value is 'Asia Pacific'. compileSql validates dimension names, never values. The example is fixed and the region values are enumerated. Tests now guard the budget in bytes rather than characters: em-dashes cost three bytes each and the description ran ~30 bytes over its character count. Coverage extends to every parameter and to query_lfx_lens, which has far less headroom and is the likelier of the two to drift past the cut unnoticed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Josep Garcia-Reyero Sais <josepreyero@gmail.com>
Contributor
Author
|
Superseded by #109. Same commits, branch renamed to |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
internal/tools/lens.go:175
- This now advertises sponsorship metrics in the semantic layer, but
query_lfx_lensstill says to always route event sponsorships there because the semantic layer only handles non-sponsorship event data (internal/tools/lens.go:50). These conflicting tool descriptions can send clients to the wrong backend. Remove sponsorship here unless support/routing has changed, in which case the other tool description must be updated in this PR too.
- events — event, registration, speaker, sponsorship counts and revenue
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.
Follow-up to #107, from testing the guidance against the real dbt Semantic Layer API and a live MCP session.
A wrong dimension name shipped in #107. The organization HQ dimension is entity-prefixed —
activity_project_id__organization_lf_region— not the bareorganization_lf_regionthe tip claimed. A client copying that would build an invalid query. Verified by queryingdimensionsPaginateddirectly and by running the grouped query end to end.The empty-result trap. A real client session searched
list_metricsfor "contributor organization country" and got[], then abandoned the semantic layer entirely and fell back to the SQL agent. Measured against the API:search: "country"returns 0 metrics,"region"returns 0,"contributor"returns 2. Country and region are dimensions — they are not searchable vialist_metricsby design, and nothing said so.So this states what metrics and dimensions each are, that
list_metricsmatches metric names and descriptions only, and that regional dimensions come fromget_dimensionson a metric found by topic.Validated end to end against a locally built lens (main, with linuxfoundation/lfx-lens#25 and #26) through a local MCP server: discover
total_contributorsby topic →get_dimensions→ global query grouped bycountry__lf_regionwith no project scope, returning real data.🤖 Generated with Claude Code