- 
                Notifications
    
You must be signed in to change notification settings  - Fork 28
 
Add cross-product uuid to all telemetry events #1619
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
| "dependencies": { | ||
| "@hediet/std": "^0.6.0", | ||
| "@vscode/extension-telemetry": "^0.4.10", | ||
| "appdirs": "^1.1.0", | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[F] Doesn't even have a types file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Appears this NPM port also doesn't support Windows
It currently supports OS X and Unix operating systems (Unix support is according to the XDG specification). Windows support is not yet implemented, but a pull request adding that would be greatly appreciated!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for cross-referencing from Notion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've looked at the source code for both the Python and Javascript implementations.
For windows in Python we have:
    if system == "win32":
        if appauthor is None:
            appauthor = appname
        const = roaming and "CSIDL_APPDATA" or "CSIDL_LOCAL_APPDATA"
        path = os.path.normpath(_get_win_folder(const))
        if appname:
            if appauthor is not False:
                path = os.path.join(path, appauthor, appname)
            else:
                path = os.path.join(path, appname)
...
    return path
For JS:
var appendNameVersion = function (dir, appname, version) {
  if (appname) {
    dir = path.join(dir, appname);
    if (version) {
      dir = path.join(dir, version);
    }
  }
  return dir;
};
...
  userConfigDir: function (appname, appauthor, version, roaming) {
    var dir = roaming ? process.env.APPDATA : process.env.LOCALAPPDATA;
    return appendNameVersion(dir, appname, version);
  }
so if process.env.APPDATA is correct and provided we should get the desired result by calling userConfigDir(join('iterative', 'telemetry') (spec here)
f5c2083    to
    bfe183f      
    Compare
  
            
          
                extension/src/telemetry/uuid.ts
              
                Outdated
          
        
      | user_id: string | ||
| } | ||
| 
               | 
          ||
| const readOrCreateConfig = (): UserConfig | undefined => { | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From the spec
from shutil import copy
from pathlib import Path
from appdirs import user_config_dir
# DVC backwards-compatibility
old = Path(user_config_dir("dvc/user_id", "iterative"))
# cross-product path
new = Path(user_config_dir("iterative/telemetry", False))
if old.exists() and not new.exists():
    copy(old, new)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This has been updated to:
import json
from shutil import copy
from pathlib import Path
from appdirs import user_config_dir
# DVC backwards-compatibility
old = Path(user_config_dir("dvc/user_id", "iterative"))
# cross-product path
new = Path(user_config_dir("iterative/telemetry", False))
uid = generate_id() # see above
if new.exists():
    uid = new.read_text().strip()
if old.exits():
    uid = json.load(old.open())["user_id"]
    new.write_text(uid)
else old.exists():  # only for non-DVC packages:
    # write legacy file in case DVC is installed later
    old.write_text(f'{{"user_id": "{uid}"}}')
Pretty sure the last statement should be just else.
8d5ff5f    to
    bd31e53      
    Compare
  
    | 
           Please re-review so we can clear this one down.  | 
    
| 
           Code Climate has analyzed commit a37afa3 and detected 0 issues on this pull request. The test coverage on the diff in this pull request is 90.7% (85% is the threshold). This pull request will bring the total coverage in the repository to 96.8% (-0.1% change). View more on Code Climate.  | 
    
This PR adds the creation/loading of a company-wide telemetry UUID to our analytics calls for cross-product analytics.
Have a look at the notion spec and LMK if you think I have missed anything.