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

add tsserver version property to every event #37066

Merged
merged 5 commits into from Oct 30, 2017

Conversation

Projects
None yet
3 participants
@aozgaa
Contributor

aozgaa commented Oct 28, 2017

Adds a property to all typescript telemetry events indicating the version of tsserver in the currently running instance of tsserver.

There are two sources of the version of tsserver:

  1. the API version: this is the version that the version provider gets from the package.json at the root of the tsserver path.
  2. tsserver's reported version: the server returns its internal version string as part of the 'projectInfo' event. This event is only fired by tsserver if it was started with --enableTelemetry, which only occurs if we decided the APIVersion was >=2.0.8. If we could recover the version from the package.json, then tsserver's reported version should match the API Version unless tsserver was overwritten.

Since the tsserver version is already reported (for recent tsserver versions), we could in principle join the session of an event we are interested in, say when a crash or exception occurs in tsserver, to the corresponding projectInfo event and recover the version of tsserver running. However, this has a couple issues:

  1. Events with versions are easily filterable/queryable. This is part of why versioning is considered a good practice.
  2. If the crash happens before the projectInfo event fires, we get no version info.
  3. If tsserver restarts, the version of tsserver running might change. It may be difficult to disambiguate which event corresponds to which instance of tsserver without many joins, yielding a slow query.
  4. It is easier to query

Open questions

  1. Should there be distinct properties for distinct version sources, or a single property picking the more reliable of the two?
  • I'm not strongly partial either way. It is a bit inefficient to include versions that will almost always agree on every event, but may be useful to see both when there is a discrepancy. We could also only tack on a second property when they disagree.
  1. Should we reuse the property name version from projectinfo?
  • Note that version is a term that could potentially be overloaded. On the other hand, version is already used by the projectInfo event, and we should (and in this implmentation, do) add the same property to every event, including projectInfo. In the PR, the version property we append to every event will coincide exactly with the version from projectInfo.

TODO

  • figure out how to annotate the property correctly for GDPR categorization, and validate it.
@aozgaa

This comment has been minimized.

Show comment
Hide comment
@aozgaa

aozgaa Oct 30, 2017

Contributor

This test was failing locally (Ubuntu 17.04) on some ipc tests, but the same tests are failing in master.

Contributor

aozgaa commented Oct 30, 2017

This test was failing locally (Ubuntu 17.04) on some ipc tests, but the same tests are failing in master.

Show outdated Hide outdated extensions/typescript/src/utils/telemetry.ts Outdated
if (telemetryData.telemetryEventName === 'projectInfo') {
this._tsserverVersion = properties['version'];
}
/* __GDPR__
"typingsInstalled" : {

This comment has been minimized.

@mjbvz

mjbvz Oct 30, 2017

Contributor

The version field will need to be added to the gdpr annotations as well.

@kieferrm Would __GDPR__COMMON__ work for this?

@mjbvz

mjbvz Oct 30, 2017

Contributor

The version field will need to be added to the gdpr annotations as well.

@kieferrm Would __GDPR__COMMON__ work for this?

This comment has been minimized.

@kieferrm

kieferrm Oct 31, 2017

Contributor

GDPR__COMMON means that this is a property that is on every single telemetry event we send. The TS server version is only on TS related events, so it is not a common property.

@kieferrm

kieferrm Oct 31, 2017

Contributor

GDPR__COMMON means that this is a property that is on every single telemetry event we send. The TS server version is only on TS related events, so it is not a common property.

This comment has been minimized.

@aozgaa

aozgaa Oct 31, 2017

Contributor

Maybe this can be done with GDPR__FRAGMENT and a wildcard to match on the name? Or would we need to extend the comment-handling functionality?

@aozgaa

aozgaa Oct 31, 2017

Contributor

Maybe this can be done with GDPR__FRAGMENT and a wildcard to match on the name? Or would we need to extend the comment-handling functionality?

@mjbvz mjbvz added this to the October 2017 milestone Oct 30, 2017

@mjbvz mjbvz merged commit 85e479f into Microsoft:master Oct 30, 2017

2 of 3 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
license/cla All CLA requirements met.
Details
@mjbvz

This comment has been minimized.

Show comment
Hide comment
@mjbvz

mjbvz Oct 30, 2017

Contributor

Thanks. Getting this in for our October release

Contributor

mjbvz commented Oct 30, 2017

Thanks. Getting this in for our October release

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment