Skip to content

Support metrics_sql in canvas transitive access resolution#9023

Merged
begelundmuller merged 3 commits intomainfrom
begelundmuller/canvas-metrics-sql-transitive-access
Mar 13, 2026
Merged

Support metrics_sql in canvas transitive access resolution#9023
begelundmuller merged 3 commits intomainfrom
begelundmuller/canvas-metrics-sql-transitive-access

Conversation

@begelundmuller
Copy link
Contributor

@begelundmuller begelundmuller commented Mar 11, 2026

  • Adds metrics_sql property lookup in populateRendererRefs, using a metrics_sql resolver to discover referenced metrics views via Refs()
  • Uses SkipChecks: true when initializing the resolver to avoid infinite recursion through the security engine

Checklist:

  • Covered by tests
  • Ran it and it works as intended
  • Reviewed the diff before requesting a review
  • Checked for unhandled edge cases
  • Linked the issues it closes
  • Checked if the docs need to be updated. If so, create a separate Linear DOCS issue
  • Intend to cherry-pick into the release branch
  • I'm proud of this work!

@begelundmuller begelundmuller self-assigned this Mar 11, 2026
@begelundmuller begelundmuller requested a review from pjain1 March 12, 2026 15:01
}

// metricsSQL parses and registers metrics view references found in a metrics SQL string.
func (r *rendererRefs) metricsSQL(ctx context.Context, sql any) error {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we parse metrics sql during reconcile, so when this is will be rendered and sql is wrong like column got deleted etc. then it will fail the entire canvas instead of just that component. Earlier I think if only a metrics view had some issue then only that component will fail.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great point! Yes we definitely don't want to fail the whole canvas in that case.

I updated the PR with a generic solution, does that look good?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good

@begelundmuller begelundmuller merged commit 881cf37 into main Mar 13, 2026
15 of 16 checks passed
@begelundmuller begelundmuller deleted the begelundmuller/canvas-metrics-sql-transitive-access branch March 13, 2026 14:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants