Skip to content

feat(scm-multi-platform-detetion): Extending multi platform detector functionality#117444

Merged
Abdkhan14 merged 9 commits into
masterfrom
abdk/multi-platform-detection-v7
Jun 12, 2026
Merged

feat(scm-multi-platform-detetion): Extending multi platform detector functionality#117444
Abdkhan14 merged 9 commits into
masterfrom
abdk/multi-platform-detection-v7

Conversation

@Abdkhan14

@Abdkhan14 Abdkhan14 commented Jun 11, 2026

Copy link
Copy Markdown
Contributor

What's novel:

Top-N language selection_select_active_platforms (L1078) The old detector picks one language. This selects up to MAX_LANGUAGES=3 distinct base platforms by byte count, grouping related languages (TypeScript + JavaScript → javascript). It's what enables detecting a Next.js frontend and Django backend in the same run.

Recursive tree fetch and index_get_tree, _build_tree_index, _TreeIndex (L1187–L1249) The old detector only looks at the root listing. These fetch the full recursive git tree once and index every file and directory basename across the whole repo, making framework config files detectable anywhere in the tree — not just at root.

Noise-scoping ignore list_IGNORED_TREE_SEGMENTS, _path_is_ignored (L1123–L1184) A whole-tree index introduces false positives: node_modules/express/package.json would contribute express as a basename signal. The ignore list drops any tree entry whose path passes through a vendored or build directory, which the root-only detector never needed.

K-reads accounting_collect_needed_paths (L1270) Instead of fetching content eagerly, Tier 1 skips all reads and counts what would be needed to resolve content/package rules. This is emitted as k_reads_needed and is the key input for sizing the Tier 2 read budget before committing to it.

Confidence-aware sort_CONFIDENCE_ORDER (L1044) With multiple base platforms in one run, a bare-language medium fallback (e.g. javascript) could outsort a high-confidence framework from a smaller language (e.g. python-django) purely on byte count. Adding confidence as the primary sort key prevents that.

MultiDetectionResult (L1252) Wraps the platform list with measurement scaffolding — k_candidate, is_truncated, tree_entry_count, repo_size_bytes — that feeds the harness and metrics without leaking into the return type of the eventual live detector. This is just used by the test script. These fields are explicitly temporary and will be stripped once thresholds are set.

@Abdkhan14 Abdkhan14 requested review from a team as code owners June 11, 2026 18:17
@github-actions github-actions Bot added the Scope: Backend Automatically applied to PRs that change backend components label Jun 11, 2026
@Abdkhan14 Abdkhan14 changed the title Abdk/multi platform detection v7 feat(scm-multi-platform-detetion): Extending multi platform detector functionality Jun 11, 2026
Comment thread src/sentry/integrations/github/multi_platform_detection.py
Comment thread src/sentry/integrations/github/multi_platform_detection.py
# Language → platform mapping (GitHub Linguist names → Sentry base platform IDs)
# ---------------------------------------------------------------------------

GITHUB_LANGUAGE_TO_SENTRY_PLATFORM: dict[str, str] = {

@jaydgoss jaydgoss Jun 11, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Seems that a lot of this is duped relative to src/sentry/integrations/github/platform_detection.py, I think it would be beneficial in terms of understanding the diff, preventing drift, and understanding which aspects are unique to the two variants to centralize some of this.

You could create a third module (e.g. platform_registry.py) holding the shared layer, imported by both:

  • Constants/data: GITHUB_LANGUAGE_TO_SENTRY_PLATFORM, IGNORED_LANGUAGES, FRAMEWORKS, derived indexes, _NON_SELECTABLE_PLATFORMS, _PACKAGE_MANIFEST_FILES
  • TypedDicts: DetectedPlatform, DetectorRule, FrameworkDef, _PackageManifest
  • Core matching: _rule_matches, _framework_matches, _apply_supersession

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yup, I'll do that real quick

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@jaydgoss this is done ^

)


def _path_is_ignored(path: str) -> bool:

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Might not make sense to do @ this stage, but it could be helpful to add tests for some of the new code, would at least tell us we aren't accidentally breaking measurement results via a pure function not working as expected.

Could add tests for: _path_is_ignored, _build_tree_index, _select_active_platforms, _collect_needed_paths, detect_platforms_multi (fake client, a few cases: subdir next.config.js in apps/web/ detected, confidence-aware sort (a small python-django high beats a larger javascript medium), empty tree, truncated: true surfaced).

@Abdkhan14 Abdkhan14 Jun 11, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I'll follow up with a pr with test files for this, I'll add tests for the new branches

@Abdkhan14 Abdkhan14 requested a review from jaydgoss June 11, 2026 19:35
basename = path.rsplit("/", 1)[-1]

if entry_type == "blob":
files.add(basename)

@jaydgoss jaydgoss Jun 12, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

files is a set of basenames, so a monorepo with 30 package.jsons reports k_reads_needed = 1. The high-read repos are the ones this metric is meant to find, and the dedup makes them all look small. Tier 2 needs full paths to fetch contents anyway, so could store full paths and keep a basename lookup for the existence rules (also gives us subdir attribution later).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Makes full sense, I updated the code to consider the full paths, pushed

for fw in _FRAMEWORKS_BY_PLATFORM.get(base_platform, []):
# Pass empty file_contents and no manifest so only path/dir/ext
# existence rules fire; content/package rules return False here.
if _framework_matches(fw, index.files, {}, None, index.dirs):

@jaydgoss jaydgoss Jun 12, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Whole-tree basename matching loses co-location, so every rules can be satisfied across unrelated subprojects: a stray .csproj in tools/ plus an appsettings.json anywhere reads as dotnet-aspnetcore high, and the ignore list can't catch it. Non-blocking for measurement, but worth a test repo with that shape.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yup, makes sense, I've updated the local plan file I have to track the project. Will take into account when implementing the next stage

platform=platform_id,
language=language,
bytes=byte_count,
confidence="high",

@jaydgoss jaydgoss Jun 12, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Package/content-only frameworks can't match here, so existence siblings win: an RN repo comes back apple-ios high via ios/*.xcodeproj while react-native is invisible. Nothing to change in this PR, but once Tier 2 reads land, escalation shouldn't stop on a high match while needed_paths is non-empty.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Makes sense, updated my local impl plan file with this comment

# languages (e.g. TS after JS) are grouped for free.
count += 1
if count > MAX_LANGUAGES:
break

@jaydgoss jaydgoss Jun 12, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should this be continue? Once a 4th platform shows up, later related languages stop grouping (byte order TS, Python, Go, C#, JS: the JS bytes never join the javascript bucket), which skews the bytes sort.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

yeah, makes sense, changed to continue, pushed


if entry_type == "blob":
files.add(basename)
repo_size_bytes += entry.get("size") or 0

@jaydgoss jaydgoss Jun 12, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This used to sum every blob, now only non-ignored ones, so the existing series quietly changes meaning. Full size is also what the Tier 4 archive decision needs (the tarball includes vendored dirs). Maybe emit both?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yep, emitting full_repo_size_bytes now fwiw

Comment thread src/sentry/integrations/github/multi_platform_detection.py Outdated
count += 1
if count > MAX_LANGUAGES:
continue
active_platforms[base_platform].append((language, byte_count))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Fourth base platform dropped entirely

Medium Severity

_select_active_platforms skips any language whose base platform is not already active once three distinct base platforms have been counted. If JavaScript is the first language mapped to javascript but appears only after three other base platforms, its bytes are never added and that stack is omitted from detection entirely.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit b5f90fa. Configure here.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

break -> continue fixed this

Comment thread src/sentry/integrations/github/multi_platform_detection.py
@Abdkhan14 Abdkhan14 requested a review from jaydgoss June 12, 2026 16:27
Comment thread src/sentry/integrations/github/multi_platform_detection.py

@cursor cursor Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 1 potential issue.

There are 2 total unresolved issues (including 1 from previous review).

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 3103f3e. Configure here.

if entry_type == "blob":
full_paths_by_basename[basename].add(path)
elif entry_type == "tree":
dirs.add(basename)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Unity dirs need same project root

Medium Severity

_build_tree_index stores directory names as a flat set of basenames, and tier‑1 framework matching passes that set into _framework_matches. Unity’s every rule only checks that Assets and ProjectSettings exist somewhere in the repo, not under the same project root, so unrelated folders with those names can yield a false high‑confidence Unity hit in detect_platforms_multi.

Additional Locations (1)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 3103f3e. Configure here.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah, taken into account

Comment thread src/sentry/integrations/github/multi_platform_detection.py
for fw in _FRAMEWORKS_BY_PLATFORM.get(base_platform, []):
# Pass empty file_contents and no manifest so only path/dir/ext
# existence rules fire; content/package rules return False here.
if _framework_matches(fw, index.files, {}, None, index.dirs):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

index.files builds a new set from all basenames on every access, and this loop reads it once per framework (35 times for a javascript bucket). Computing it once into a local before pass 1 avoids rebuilding it against a potentially 100k entry tree.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yup, will add this change

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done

@Abdkhan14 Abdkhan14 merged commit f34f57d into master Jun 12, 2026
64 checks passed
@Abdkhan14 Abdkhan14 deleted the abdk/multi-platform-detection-v7 branch June 12, 2026 18:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Scope: Backend Automatically applied to PRs that change backend components

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants