Skip to content

fix(vscode): Fix log subscriptions telemetry#8858

Open
andrew-eldridge wants to merge 3 commits intomainfrom
aeldridge/subscriptionsTelemetryFix
Open

fix(vscode): Fix log subscriptions telemetry#8858
andrew-eldridge wants to merge 3 commits intomainfrom
aeldridge/subscriptionsTelemetryFix

Conversation

@andrew-eldridge
Copy link
Contributor

@andrew-eldridge andrew-eldridge commented Feb 26, 2026

Commit Type

  • feature - New functionality
  • fix - Bug fix
  • refactor - Code restructuring without behavior change
  • perf - Performance improvement
  • docs - Documentation update
  • test - Test-related changes
  • chore - Maintenance/tooling

Risk Level

  • Low - Minor changes, limited scope
  • Medium - Moderate changes, some user impact
  • High - Major changes, significant user/system impact

What & Why

Unawaited async logSubscriptions function doesn't log telemetry before startup completes

Impact of Change

  • Users: N/A
  • Developers: Fixes missing subscription telemetry, now in azureLogicAppsStandard.logSubscriptions event, separate from activate
  • System: N/A

Test Plan

Minor change to context for subscription telemetry, does not require additional tests

  • Unit tests added/updated
  • E2E tests added/updated
  • Manual testing completed
  • Tested in:

Contributors

@andrew-eldridge

@andrew-eldridge andrew-eldridge added risk:low Low risk change with minimal impact VSCode Issues or PRs specific to VS Code extension bug Something isn't working labels Feb 26, 2026
@github-actions
Copy link

github-actions bot commented Feb 26, 2026

🤖 AI PR Validation Report

PR Review Results

Thank you for your submission! Here's detailed feedback on your PR title and body compliance:

PR Title

  • Current: fix(vscode): Fix log subscriptions telemetry
  • Issue: None — title is concise, follows conventional commit style, and clearly describes the change.
  • Recommendation: No change needed.

Commit Type

  • Properly selected (fix).
  • Only one commit type selected which is correct.

Risk Level

  • The PR lists Low and the repo labels include risk:low.
  • Assessment: Matches the code changes (small, 5 additions / 2 deletions across 3 files, no public API changes). Low risk is appropriate.

What & Why

  • Current: Unawaited async logSubscriptions function doesn't log telemetry before startup completes
  • Issue: None — concise and focused on the actual problem.
  • Recommendation: No change required. (Optional: you can add the function name/file path to be extra explicit, e.g. "Fix unawaited apps/vs-code-designer/src/app/utils/telemetry.ts: logSubscriptions".)

Impact of Change

  • Impact section is present and concise.
  • Recommendation: Consider adding a one-line note to the Developers section about the specific module/files touched to make it easier for maintainers to locate the change:
    • Users: N/A
    • Developers: Affects VSCode extension telemetry handling in apps/vs-code-designer (telemetry.ts, main.ts)
    • System: N/A

Test Plan

  • Manual testing is selected and you included a short justification: "Minor change to context for subscription telemetry, does not require additional tests." This is acceptable given the small scope.
  • Recommendation: Add 1-2 short steps describing how the manual test was performed (for traceability). Example:
    • Open extension, observe telemetry event azureLogicAppsStandard.logSubscriptions emitted during startup (verify properties contain subscriptions), and confirm no startup delay/regressions.

Contributors

  • Contributors listed (author @andrew-eldridge). Good — credit provided.
  • Recommendation: If others contributed reviews or design input, optionally add them too.

Screenshots/Videos

  • Not applicable for this non-visual change. This is fine.

Summary Table

Section Status Recommendation
Title No change needed.
Commit Type No change needed.
Risk Level No change needed.
What & Why No change needed (optional: include file name).
Impact of Change Add a short note naming affected files/modules.
Test Plan Add 1-2 manual test steps used.
Contributors No change needed (optional: add other reviewers if applicable).
Screenshots/Videos Not applicable.

Final message
Please remove the needs-pr-update label if there are no follow-up changes required by your CI/workflow — it currently suggests the PR still needs edits. Otherwise, make the follow-ups suggested above (adding brief manual test steps and optionally naming the files in Impact) and re-run/confirm any related checks. The advised risk level is Low (matches your selection).

Thank you for the clear PR — small focused change and well-documented.


Last updated: Tue, 03 Mar 2026 18:32:58 GMT

@github-actions
Copy link

🤖 AI PR Validation Report

PR Review Results

Thank you for your submission! Here's detailed feedback on your PR title and body compliance:

PR Title

  • Current: fix(vscode): Startup await log subscriptions
  • Issue: Title is generally fine and follows conventional commit style. Minor wording suggestion to clarify intent.
  • Recommendation: Consider a slightly clearer phrasing such as: fix(vscode): await subscription telemetry during startup or fix(vscode): await logSubscriptions during startup to make the subject unambiguous.

Commit Type

  • No commit type checkbox is selected in the PR body.
  • Only one should be selected. For this change, the correct type is fix.
  • Recommendation: In the PR body check the fix - Bug fix box.

Risk Level

  • Assessment: The PR currently has no risk-level checkbox selected in the body, but the PR has a risk:low label.
  • Issue: The PR body must include exactly one risk level selection and it must match the repository label. Right now there is a mismatch (no selection vs label risk:low).
  • Recommendation: Select Low - Minor changes, limited scope in the PR body to match the risk:low label.

⚠️ What & Why

  • Current: "Unawaited async logSubscriptions function doesn't log telemetry before startup completes"
  • Issue: The statement is concise but could be clearer and include what was changed and why (what behaviour is fixed and why this matters).
  • Recommendation: Expand to a one-paragraph description that includes the before/after and reason. Example:
    • "Before: logSubscriptions was invoked but not awaited, which could cause subscription telemetry not to be recorded during startup. After: start the logSubscriptions call earlier and await it at the appropriate point during activation so subscription telemetry is reliably logged during startup. This ensures startup telemetry includes subscription information."

⚠️ Impact of Change

  • Issue: The Impact section exists but is a little unclear and contains statements that may be misleading (e.g., "Will see increase in startup times"). The code change slightly alters startup ordering by awaiting an already-fired promise, which may have minor timing effects but primarily fixes missing telemetry.
  • Recommendation: Clarify impacts and be specific. Example bullets to include in the PR body:
    • Users: No direct functional changes; startup may be negligibly affected but telemetry will be more complete.
    • Developers: Subscription telemetry will now be recorded during startup — helpful for diagnostics.
    • System: No significant performance impact expected. If you measured an effect, add numbers.

Test Plan

  • Assessment: No unit or E2E tests were added, and the Test Plan checkboxes are unchecked with no explanation.
  • Issue: This is a code change that affects startup ordering and telemetry; the PR should either include tests or a clear, justified reason why tests are not necessary. The repository policy here requires checking that tests claimed are actually present in the diff.
  • Recommendation: Do one of the following:
    • Add unit tests that verify logSubscriptions is awaited or that telemetry contains subscriptions after activation, or
    • Add an integration/E2E test that covers startup telemetry, or
    • If adding tests is impractical, update the Test Plan to include "Manual testing completed" and provide explicit steps and results showing telemetry behavior before/after (include environment and reproduction steps). Then check the appropriate box(es) in the Test Plan section.

⚠️ Contributors

  • Assessment: Contributors section lists @andrew-eldridge.
  • Recommendation: Good — keep any additional reviewers/contributors listed if applicable. If no other contributors, this is fine.

Screenshots/Videos

  • Assessment: Not applicable for this change (no UI changes). No action needed.

Summary Table

Section Status Recommendation
Title Minor wording suggestion (optional)
Commit Type Select fix in the PR body
Risk Level Select Low in PR body to match label risk:low
What & Why ⚠️ Expand to explain before/after and why it matters
Impact of Change ⚠️ Clarify users/developers/system impact; remove ambiguous phrasing
Test Plan Add unit/E2E tests or provide a detailed manual test plan and check appropriate boxes
Contributors ⚠️ @andrew-eldridge present — add others if needed
Screenshots/Videos Not applicable

Final message:
This PR does not pass the PR body/template checks. The code change is small and risk appears low (matches the risk:low label), but the PR body is missing required selections (Commit Type and Risk Level) and does not include tests or an adequate test justification. Please update the PR body to:

  1. Select exactly one Commit Type — check fix.
  2. Select exactly one Risk Level — check Low to match the risk:low label.
  3. Expand the "What & Why" with a short before/after description (see the recommended phrasing above).
  4. Clarify the "Impact of Change" using explicit bullets for Users/Developers/System.
  5. Update the Test Plan:
    • Add unit or E2E tests that cover the behavior OR
    • If not possible, add a clear manual test plan with reproduction steps and observed results, then check Manual testing completed.

Once those changes are made, re-request review. The advised risk level based on the diff is low and matches the PR label, so no change to the risk label is required.

Thank you for the clear, small diff — once the PR body is updated and tests or justification are added this should be good to merge.


Last updated: Thu, 26 Feb 2026 17:14:27 GMT

@github-actions
Copy link

github-actions bot commented Feb 26, 2026

📊 Coverage Check

The following changed files need attention:

apps/vs-code-designer/src/main.ts - 0% covered

⚠️ apps/vs-code-designer/src/app/utils/telemetry.ts - 71% covered (needs improvement)

Please add tests for the uncovered files before merging.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR fixes a bug where the async logSubscriptions function was called without being awaited during VS Code extension activation, potentially causing subscription telemetry to not be captured before the activation event completed.

Changes:

  • Converted fire-and-forget logSubscriptions call to a promise pattern that starts early and awaits at the end of activation
  • Removed unnecessary async keyword from logExtensionSettings function (it never used await internally)

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
apps/vs-code-designer/src/main.ts Starts logSubscriptions promise early (line 115) and awaits it at end of activation (line 163) to ensure telemetry is captured
apps/vs-code-designer/src/app/utils/telemetry.ts Removes async keyword from logExtensionSettings since it's synchronous

@andrew-eldridge andrew-eldridge changed the title fix(vscode): Startup await log subscriptions fix(vscode): Fix log subscriptions telemetry Mar 3, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working pr-validated risk:low Low risk change with minimal impact VSCode Issues or PRs specific to VS Code extension

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants