iTwin / itwinjs-core

Monorepo for iTwin.js Library
https://www.itwinjs.org
MIT License
606 stars 210 forks source link

Remove the Telemetry APIs (aka @itwin/core-telemetry) #5722

Open calebmshafer opened 1 year ago

calebmshafer commented 1 year ago

Completely remove the @itwin/core-telemetry package and the IModelHost.telemetry and IModelApp.telemetry APIs.

Background

The Telemetry APIs exposed have been used for feature tracking and not any of the other types of telemetry. We mostly use our Logger classes and, as of 4.0, are starting to introduce OpenTelemetry for the other types of telemetry (such as trace events). Therefore it never caught on for broader use outside of feature tracking.

Feature tracking, in general, is an application-level concept as many assumptions need to be made on what is important for your application to track, where to track it and how to identify/distinguish the different features you will be tracking.

During the iTwin.js 3.0 release, we removed a lot of the assumptions in our telemetry APIs by removing any indication of where to track it (for example Application Insights and Bentley internal feature tracking) but did not go all the way to removing the full use of it. However, we did take a small step and mark the APIs in @itwin/core-telemetry as alpha and marked the use in IModelHost and IModelApp as internal to discourage usage until we decided what to do.

The usage of the API persisted into iTwin.js 4.0 release mostly as a feature tracking API, specifically a few AppUI cases (removed https://github.com/iTwin/appui/pull/397) and when an iModel was opened (removed https://github.com/iTwin/itwinjs-core/pull/5721). Early in the 4.x releases we removed the rest of the usage of the API but did not remove it due to the breaking changes it would cause despite them being internal/alpha APIs at the time.

Path forward

With the stance that feature tracking is an application-level concept, we will remove the current usage of the Telemetry APIs in all libraries with the anticipation of removal in 5.0.

As for the recommendation to applications building on iTwin.js, the libraries should be raising events for all cases where the applications would like to be notified of something happening. If there is not a notification today, we should determine the best way to raise one for their consumption. Please file an issue requesting such an event or notification.

pmconne commented 1 year ago

Do we know what other internal/alpha APIs appui is using? If we can't remove any such API without causing a breaking change for them, we need a list, and we probably should add prominent documentation to those APIs so we don't accidentally remove them.

I thought I recalled when the appui repo was created they ran the no-internal lint rule and addressed all the problems. Our API policies are a two-way street - you don't get the stability guarantees if you don't follow the usage rules.

calebmshafer commented 1 year ago

@pmconne I do not, it was initially prior to the 4.0 release but I do not know what still exists after the release of 4.0 in both sets of packages.

@aruniverse @raplemie @GerardasB @ben-polinsky can we please run it and see what still exists? We need to come up with a plan for each such instance.

pmconne commented 1 year ago

can we please run it

They should have the no-internal rule enabled as part of their base eslint config (but they don't appear to).

pmconne commented 1 year ago

List of unique internal+alpha APis in use:

class "Constant" is alpha.
class "IModelReadRpcInterface" is internal.
class "SubCategoriesCache" is internal.
class "TelemetryEvent" is alpha.
class "TelemetryManager" is alpha.
constructor "Models" is internal.
function "matchesWords" is internal.
getter "IModelConnection.subcategories" is internal.
interface "SubCategoriesRequest" is internal.
method "UiEventDispatcher.setTimeoutPeriod" is internal.
method "UiItemsManager.clearAllProviders" is internal.
method "UiLayoutDataProvider.layoutDialogRows" is internal.
property "AccuDraw.newFocus" is internal.
property "IModelApp.telemetry" is internal.
property "RenderSystem.options" is internal.

IModelReadRpcInterface and Models are used exclusively in tests. RenderSystem.options is used for its wantShadows property but they're already checking viewport.wantShadows which checks the RenderSystem option itself. IModelConnection.categories provides access to subcategories, you don't need IModelConnection.subcategories or SubCategoriesCache. I don't know why core-geometry's Constant is alpha.

Occurrences:

==[ @itwin/imodel-components-react ]==============================[ 7 of 11 ]==

Invoking: eslint -f visualstudio "./src/**/*.{ts,tsx}" 1>&2 
imodel-components-react/navigationaids/DrawingNavigationAid.tsx(1074,48): error @itwin/no-internal : class "Constant" is alpha.

1 problem
Returned error code: 1
"@itwin/imodel-components-react" failed to build.

==[ @itwin/appui-react ]=========================================[ 11 of 11 ]==

Invoking: eslint -f visualstudio "./src/**/*.{ts,tsx}" 1>&2 
appui-react/UiFramework.ts(854,30): error @itwin/no-internal : class "TelemetryEvent" is alpha.
appui-react/UiFramework.ts(863,13): error @itwin/no-internal : class "TelemetryManager" is alpha.
appui-react/UiFramework.ts(863,13): error @itwin/no-internal : class "TelemetryManager" is alpha.
appui-react/UiFramework.ts(863,13): error @itwin/no-internal : class "TelemetryManager" is alpha.
appui-react/UiFramework.ts(863,13): error @itwin/no-internal : property "IModelApp.telemetry" is internal.
appui-react/accudraw/FrameworkAccuDraw.ts(302,48): error @itwin/no-internal : property "AccuDraw.newFocus" is internal.
appui-react/hooks/useSolarDataProvider.tsx(20,9): error @itwin/no-internal : property "RenderSystem.options" is internal.
appui-react/hooks/useSolarDataProvider.tsx(26,11): error @itwin/no-internal : property "RenderSystem.options" is internal.
appui-react/hooks/useSolarDataProvider.tsx(34,13): error @itwin/no-internal : property "RenderSystem.options" is internal.
appui-react/hooks/useSolarDataProvider.tsx(44,11): error @itwin/no-internal : property "RenderSystem.options" is internal.
appui-react/hooks/useSolarDataProvider.tsx(54,11): error @itwin/no-internal : property "RenderSystem.options" is internal.
appui-react/popup/KeyinPalettePanel.tsx(271,27): error @itwin/no-internal : function "matchesWords" is internal.
appui-react/selection/HideIsolateEmphasizeManager.ts(244,21): error @itwin/no-internal : class "SubCategoriesCache" is internal.
appui-react/selection/HideIsolateEmphasizeManager.ts(244,21): error @itwin/no-internal : class "SubCategoriesCache" is internal.
appui-react/selection/HideIsolateEmphasizeManager.ts(244,21): error @itwin/no-internal : class "SubCategoriesCache" is internal.
appui-react/selection/HideIsolateEmphasizeManager.ts(244,21): error @itwin/no-internal : getter "IModelConnection.subcategories" is internal.
appui-react/selection/HideIsolateEmphasizeManager.ts(245,24): error @itwin/no-internal : interface "SubCategoriesRequest" is internal.
appui-react/selection/HideIsolateEmphasizeManager.ts(249,9): error @itwin/no-internal : class "SubCategoriesCache" is internal.
appui-react/selection/HideIsolateEmphasizeManager.ts(249,9): error @itwin/no-internal : class "SubCategoriesCache" is internal.
appui-react/selection/HideIsolateEmphasizeManager.ts(249,9): error @itwin/no-internal : class "SubCategoriesCache" is internal.
appui-react/selection/HideIsolateEmphasizeManager.ts(249,9): error @itwin/no-internal : getter "IModelConnection.subcategories" is internal.
appui-react/syncui/SyncUiEventDispatcher.ts(84,5): error @itwin/no-internal : method "UiEventDispatcher.setTimeoutPeriod" is internal.
appui-react/syncui/SyncUiEventDispatcher.ts(84,5): error @itwin/no-internal : method "UiEventDispatcher.setTimeoutPeriod" is internal.
appui-react/syncui/SyncUiEventDispatcher.ts(84,5): error @itwin/no-internal : method "UiEventDispatcher.setTimeoutPeriod" is internal.
appui-react/toolsettings/DefaultToolSettingsProvider.tsx(55,9): error @itwin/no-internal : method "UiLayoutDataProvider.layoutDialogRows" is internal.
appui-react/toolsettings/DefaultToolSettingsProvider.tsx(55,9): error @itwin/no-internal : method "UiLayoutDataProvider.layoutDialogRows" is internal.
appui-react/toolsettings/DefaultToolSettingsProvider.tsx(55,9): error @itwin/no-internal : method "UiLayoutDataProvider.layoutDialogRows" is internal.
appui-react/ui-items-provider/AbstractUiItemsManager.ts(101,12): error @itwin/no-internal : method "UiItemsManager.clearAllProviders" is internal.
appui-react/ui-items-provider/AbstractUiItemsManager.ts(101,12): error @itwin/no-internal : method "UiItemsManager.clearAllProviders" is internal.
appui-react/ui-items-provider/AbstractUiItemsManager.ts(101,12): error @itwin/no-internal : method "UiItemsManager.clearAllProviders" is internal.
test/content/SavedViewLayout.test.tsx(65,19): error @itwin/no-internal : getter "IModelConnection.subcategories" is internal.
test/content/SavedViewLayout.test.tsx(66,20): error @itwin/no-internal : class "SubCategoriesCache" is internal.
test/content/SavedViewLayout.test.tsx(69,20): error @itwin/no-internal : constructor "Models" is internal.
test/content/SavedViewLayout.test.tsx(84,25): error @itwin/no-internal : class "IModelReadRpcInterface" is internal.
test/content/SavedViewLayout.test.tsx(84,25): error @itwin/no-internal : class "IModelReadRpcInterface" is internal.
test/content/SavedViewLayout.test.tsx(84,25): error @itwin/no-internal : class "IModelReadRpcInterface" is internal.

36 problems
Returned error code: 1
"@itwin/appui-react" failed to build.