Make telemetry use platform only. #10437
Conversation
|
Did telemtry.md get removed completely? I think Soojin still finds that file useful |
|
There's also a husky pre-commit check that generates that file, maybe that ran and deleted everything after this change? |
TELEMETRY.md
Outdated
| @@ -2,9498 +2,3 @@ | |||
|
|
|||
| Expand each section to see more information about that event. | |||
|
|
|||
| <details> | |||
There was a problem hiding this comment.
Yeah looks like telemetry generation is broken. You likely need to change the telemetry generator too.
There was a problem hiding this comment.
It seems the generator fails to work after moving files around, fix it now.
| @@ -95,3 +112,1229 @@ export interface IInterpreterPackages { | |||
| getPackageVersion(interpreter: PythonEnvironment, packageName: string): Promise<string | undefined>; | |||
| trackPackages(interpreterUri: InterpreterUri, ignoreCache?: boolean): void; | |||
| } | |||
|
|
|||
| export interface IEventNamePropertyMapping { | |||
There was a problem hiding this comment.
ah the telemetry generator has to look here instead of index.ts for the telemetry information. That's why telemetry.md is empty.
There was a problem hiding this comment.
See this file here:
Codecov Report
@@ Coverage Diff @@
## main #10437 +/- ##
======================================
- Coverage 70% 70% -1%
======================================
Files 478 480 +2
Lines 28956 28971 +15
Branches 4859 4859
======================================
+ Hits 20535 20540 +5
- Misses 6512 6528 +16
+ Partials 1909 1903 -6
|
| import { INotebookControllerManager } from '../types'; | ||
| import { IInstaller, Product } from '../../kernels/installer/types'; | ||
| import { IVSCodeNotebookController } from '../controllers/types'; | ||
| import { trackKernelResourceInformation } from '../../kernels/telemetry/helper'; |
There was a problem hiding this comment.
A side effect is, stuff that didn't depend on kernels, now pulls in dependency from kernels folder.
There was a problem hiding this comment.
I guess that's what we're trying to fix here, and this PR is just one of many.
There was a problem hiding this comment.
@DonJayamanne you are right. This particular module InterpreterPackageTracker needs to read INotebookControllerManager, ./src/kernels, and ./src/telemetry. The best place for it is probably out of./src/notebooks and placed in a folder where we have all the moduels that glue things together.
Re #10152
Attempt to enforce using
platformmodules only intelemetry. The PR includes two changes:notebooks/kernelIEventNamePropertyMappingwith the rest of basic helpers intelemetry. Make sure we only importplatformintelemetry, except `IEventNamePropertyMappingThe reason
IEventNamePropertyMappingis an exception is this mapping is used to track all our telemetry points and they also serve type checkingWe will check
eventValueagainstIEventNamePropertyMapping[eventName). This means current approach will have to import types from all other modules. To move off this we would need to a different approach of type checking, for examplewhich won't require us to register all the typings in
IEventNamePropertyMapping. Considering this is a rather big change and would also require changes for our tooling, I'll keep layer breaking imports in this file only.package-lock.jsonhas been regenerated by runningnpm install(if dependencies have changed).