Skip to content

improvement(client-telemetry-utils): BREAKING CHANGE: type erase ITelemetryLoggerExt#27476

Draft
jason-ha wants to merge 2 commits into
mainfrom
test/client/type-erase-ITelemetryLoggerExt
Draft

improvement(client-telemetry-utils): BREAKING CHANGE: type erase ITelemetryLoggerExt#27476
jason-ha wants to merge 2 commits into
mainfrom
test/client/type-erase-ITelemetryLoggerExt

Conversation

@jason-ha
Copy link
Copy Markdown
Contributor

@jason-ha jason-ha commented Jun 3, 2026

  • Remove deprecated methods from external ITelemetryLoggerExt and replace with branded type.
  • Retag related telemetry types as @internal and remove @deprecated tags where applicable.

@jason-ha
improvement(client-telemetry-utils): BREAKING CHANGE: type erase `ITe…
d0dc836 
…lemetryLoggerExt`

- Remove deprecated methods from external `ITelemetryLoggerExt` and replace with branded type.
- Retag related telemetry types as `@internal` and remove `@deprecated` tags where applicable.
@jason-ha jason-ha requested a review from MarioJGMsoft June 3, 2026 18:55
@jason-ha jason-ha linked an issue Jun 3, 2026 that may be closed by this pull request
Breaking Change: @fluidframework/telemetry-utils — Type-erase ITelemetryLoggerExt #26910
Open
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Jun 3, 2026 •
edited
Loading

Hi! Thank you for opening this PR. Want me to review it?

Based on the diff (227 lines, 16 files), I've queued these reviewers:

  • Correctness — logic errors, race conditions, lifecycle issues
  • Security — vulnerabilities, secret exposure, injection
  • API Compatibility — breaking changes, release tags, type design
  • Performance — algorithmic regressions, memory leaks
  • Testing — coverage gaps, hollow tests

How this works

  • Adjust the reviewer set by ticking/unticking boxes above. Reviewer toggles alone don't trigger anything.

  • Tick Start review below to dispatch the review fleet.

  • After review finishes, tick Start review again to request another run — it auto-resets after each dispatch.

  • This comment updates as new commits land; your reviewer selections are preserved.

  • Start review

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Jun 3, 2026

🔗 No broken links found! ✅

Your attention to detail is admirable.

linkcheck output


> fluid-framework-docs-site@0.0.0 ci:check-links /home/runner/work/FluidFramework/FluidFramework/docs
> start-server-and-test "npm run serve -- --no-open" 3000 check-links

1: starting server using command "npm run serve -- --no-open"
and when url "[ 'http://127.0.0.1:3000' ]" is responding with HTTP status code 200
running tests using command "npm run check-links"


> fluid-framework-docs-site@0.0.0 serve
> docusaurus serve --no-open

[SUCCESS] Serving "build" directory at: http://localhost:3000/

> fluid-framework-docs-site@0.0.0 check-links
> linkcheck http://localhost:3000 --skip-file skipped-urls.txt

Crawling...

Stats:
  290871 links
    1933 destination URLs
    2183 URLs ignored
       0 warnings
       0 errors

Josmithr
Josmithr reviewed Jun 3, 2026
View reviewed changes
Comment thread packages/utils/telemetry-utils/src/api.ts
export type {
ITelemetryErrorEventExt,
ITelemetryGenericEventExt,
// ITelemetryLoggerExt is temporarily set to an alias of `TelemetryLoggerExt` when imported from "/internal".
Copy link
Copy Markdown
Contributor

@Josmithr Josmithr Jun 3, 2026

Choose a reason for hiding this comment

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

When can this be removed? We should probably be explicit about that in the comment here.

Copy link
Copy Markdown
Contributor Author

@jason-ha jason-ha Jun 3, 2026

Choose a reason for hiding this comment

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

Whenever. Right now or later. @MarioJGMsoft has AB#74860. Likely it will happen before this PR merges, but if not, I'll add reference in here.

Copy link
Copy Markdown
Contributor

@MarioJGMsoft MarioJGMsoft Jun 4, 2026

Choose a reason for hiding this comment

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

I opened this PR to remove the code: #27484

Josmithr
Josmithr reviewed Jun 3, 2026
View reviewed changes
Comment thread packages/utils/telemetry-utils/DEV.md
Until a breaking change can be made these two types are structurally equivalent and must remain so.
The one difference is that `ITelemetryLoggerExt` has `@deprecated` methods to discourage use.
Once breaking change can be made, `ITelemetryLoggerExt` will become a branded type without any viable methods and thus neither external nor internal code may act upon it.
Temporarily, `ITelemetryLoggerExt` imported from `@fluidframework/telemetry-utils/internal` is a type alias to `TelemetryLoggerExt` to reduce internal churn.
Copy link
Copy Markdown
Contributor

@Josmithr Josmithr Jun 3, 2026

Choose a reason for hiding this comment

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

Do we have a backlog item tracking this cleanup? Would be nice if all of the relevant temporary export notes could include a TODO note referencing that item. Would make it easier to find and cleanup everything relevant when we tackle that item.

Copy link
Copy Markdown
Contributor

@MarioJGMsoft MarioJGMsoft Jun 4, 2026

Choose a reason for hiding this comment

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

I'm already doing the cleanup here: #27484

Josmithr
Josmithr reviewed Jun 3, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@Josmithr Josmithr left a comment

Choose a reason for hiding this comment

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

I left a couple comments, but overall looks good to me. Happy to review again when this is ready for merge :)

@MarioJGMsoft MarioJGMsoft mentioned this pull request Jun 4, 2026
Merged
MarioJGMsoft added a commit that referenced this pull request Jun 5, 2026
@MarioJGMsoft
(refactor): Migrate all internal ITelemetryLoggerExt use to Telemetry…
228a839 
…LoggerExt (#27484)

## Description

This change is a follow up to #26912 that introduced the new internal
`TelemetryLoggerExt` interface. Said interface can be used to replace
calls to the internal `ITelemetryLoggerExt`

## Breaking Change

This is a necessary change to enable this PR to be merged: #27476

## Reviewer Guidance

The review process is outlined on [this wiki
page](https://github.com/microsoft/FluidFramework/wiki/PR-Guidelines#guidelines).

The change is just a naming one, no functionality was affected

Fixes:
[AB#74860](https://dev.azure.com/fluidframework/235294da-091d-4c29-84fc-cdfc3d90890b/_workitems/edit/74860)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Josmithr Josmithr Josmithr left review comments

@MarioJGMsoft MarioJGMsoft Awaiting requested review from MarioJGMsoft

Copilot code review Copilot Awaiting requested review from Copilot Copilot will automatically review once the pull request is marked ready for review

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Breaking Change: @fluidframework/telemetry-utils — Type-erase ITelemetryLoggerExt

3 participants

@jason-ha @Josmithr @MarioJGMsoft