Skip to content

fix(event-log): add event logging for embedded Superset#41938

Merged
michael-s-molina merged 7 commits into
apache:6.1from
luizotavio32:cherry-pick-40083-6.1
Jul 13, 2026
Merged

fix(event-log): add event logging for embedded Superset#41938
michael-s-molina merged 7 commits into
apache:6.1from
luizotavio32:cherry-pick-40083-6.1

Conversation

@luizotavio32

@luizotavio32 luizotavio32 commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

SUMMARY

Cherry-picks #40083 (add event logging for embedded Superset) into the 6.1 branch.

This also includes the follow-up fix from #41942, which refines the embedded-dashboard detection in loggerMiddleware. The original check classified events by testing whether the URL contained the substring /embedded/, which was too broad: a regular dashboard whose slug is literally embedded (URL /dashboard/embedded/) also contains /embedded/, so its events were incorrectly tagged with source: 'embedded_dashboard' instead of source: 'dashboard'.

The substring check is replaced with a regular expression that matches only the actual embedded route shapes:

  • /dashboard/:idOrSlug/embedded/
  • /embedded/:uuid/

As a result, regular dashboard events are no longer misclassified as embedded, while genuine embedded routes continue to be detected correctly.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

N/A — internal event-logging metadata change only.

TESTING INSTRUCTIONS

Unit tests in superset-frontend/src/middleware/logger.test.ts cover source classification across paths:

  • /dashboard/123/embedded/ -> embedded_dashboard
  • /embedded/abc-def-uuid/ -> embedded_dashboard
  • /dashboard/123/ -> dashboard
  • /dashboard/embedded/ (slug is literally embedded) -> dashboard

Run:

cd superset-frontend
npm run test -- src/middleware/logger.test.ts

ADDITIONAL INFORMATION

  • Has associated issue:
  • Required feature flags:
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
    • Migration is atomic, supports rollback & is backwards-compatible
    • Confirm DB migration upgrade and downgrade tested
    • Runtime estimates and downtime expectations provided
  • Introduces new feature or API
  • Removes existing feature or API

@luizotavio32 luizotavio32 changed the title feat(event-log): add event logging for embedded Superset fix(event-log): add event logging for embedded Superset Jul 10, 2026
@dosubot dosubot Bot added the change:frontend Requires changing the frontend label Jul 10, 2026
@bito-code-review

bito-code-review Bot commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

Bito Automatic Review Skipped - Branch Excluded

Bito didn't auto-review because the source or target branch is excluded from automatic reviews.
No action is needed if you didn't intend for the agent to review it. Otherwise, to manually trigger a review, type /review in a comment and save.
You can change the branch exclusion settings here, or contact your Bito workspace admin at evan@preset.io.

@luizotavio32 luizotavio32 marked this pull request as draft July 10, 2026 17:32
Comment thread superset-frontend/src/middleware/loggerMiddleware.ts Outdated
@netlify

netlify Bot commented Jul 10, 2026

Copy link
Copy Markdown

Deploy Preview for superset-docs-preview ready!

Name Link
🔨 Latest commit b73902c
🔍 Latest deploy log https://app.netlify.com/projects/superset-docs-preview/deploys/6a512cbc6a6f88000801fa21
😎 Deploy Preview https://deploy-preview-41938--superset-docs-preview.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
🤖 Make changes Run an agent on this branch

To edit notification comments on pull requests, go to your Netlify project configuration.

@bito-code-review

Copy link
Copy Markdown
Contributor

The flagged issue is correct. Using path?.includes('/embedded/') is indeed too broad, as it will incorrectly match any URL containing the string 'embedded', including regular dashboard URLs like /dashboard/embedded/.

To resolve this, you should use a more precise check that validates the URL structure. You can use a regular expression or check for the specific path segments to ensure it only matches actual embedded routes.

Here is a concise fix using a regex to ensure /embedded/ is part of the expected path structure:

    const isEmbedded = /\/(embedded|dashboard\/.*\/embedded)\//.test(path || '');
    if (dashboardInfo?.id && (path?.includes('/dashboard/') || isEmbedded)) {
      logMetadata = {
        source: isEmbedded ? 'embedded_dashboard' : 'dashboard',
        source_id: dashboardInfo.id,
        dashboard_id: dashboardInfo.id,
        ...logMetadata,
      };
    }

There are no other comments on this PR to address. Would you like me to help with anything else?

superset-frontend/src/middleware/loggerMiddleware.ts

const isEmbedded = /\/(embedded|dashboard\/.*\/embedded)\//.test(path || '');
    if (dashboardInfo?.id && (path?.includes('/dashboard/') || isEmbedded)) {
      logMetadata = {
        source: isEmbedded ? 'embedded_dashboard' : 'dashboard',
        source_id: dashboardInfo.id,
        dashboard_id: dashboardInfo.id,
        ...logMetadata,
      };
    }

@luizotavio32 luizotavio32 force-pushed the cherry-pick-40083-6.1 branch from b73902c to e3b924f Compare July 10, 2026 18:40
@luizotavio32 luizotavio32 marked this pull request as ready for review July 10, 2026 19:14
@dosubot dosubot Bot added embedded logging Creates a UI or API endpoint that could benefit from logging. labels Jul 10, 2026
@luizotavio32 luizotavio32 marked this pull request as draft July 10, 2026 20:00
Cherry-picks the AI-suggested fix from apache#41942. The embedded-dashboard
detection classified events by checking whether the URL contained the
substring `/embedded/`, which misclassified a regular dashboard whose
slug is literally `embedded` (URL `/dashboard/embedded/`). Replace the
generic substring check with a regex that matches only the actual
embedded route shapes.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@luizotavio32 luizotavio32 marked this pull request as ready for review July 13, 2026 14:44
@dosubot dosubot Bot added the embedded label Jul 13, 2026
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@github-actions github-actions Bot removed the embedded label Jul 13, 2026
Comment thread superset-frontend/src/middleware/loggerMiddleware.ts Outdated
React Router matches the embedded routes without a trailing slash too, so
`/dashboard/:idOrSlug/embedded` and `/embedded/:uuid` are valid embedded
URLs. The regex required a trailing slash, misclassifying those events with
the wrong source. Make the trailing slash optional in both branches and add
test coverage for the no-slash variants.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@michael-s-molina michael-s-molina merged commit f8490fc into apache:6.1 Jul 13, 2026
61 checks passed
kawcl pushed a commit to CarnegieLearningWeb/superset that referenced this pull request Jul 13, 2026
Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

change:frontend Requires changing the frontend logging Creates a UI or API endpoint that could benefit from logging. size/M

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants