Skip to content
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

log active external extensions from Sourcegraph.com #5177

Open
wants to merge 15 commits into
base: master
from

Conversation

@vanesa
Copy link
Member

commented Aug 13, 2019

TODO: Update changelog and if necessary the pings documentation (https://docs.sourcegraph.com/admin/pings).

Test plan:
run localStorage.eventLogDebug="true"; in the browser console and select log levels to be Verbose. Go to any Sourcegraph page on your localhost instance and see the EVENT <name of active extension> showing the external (non-language) extensions that are currently active.

@vanesa vanesa requested a review from dadlerj Aug 13, 2019

@dadlerj
Copy link
Member

left a comment

The logging part looks good to me, but I'd check in with someone who works with the browser ext and shared code (e.g. @felixfbecker) to confirm the import works there.

@@ -3,6 +3,7 @@ import { from, Subscription } from 'rxjs'
import { bufferCount, startWith } from 'rxjs/operators'
import { ExtExtensionsAPI } from '../../extension/api/extensions'
import { ExecutableExtension, ExtensionsService } from '../services/extensionsService'
import { eventLogger } from '../../../../../web/src/tracking/eventLogger'

This comment has been minimized.

Copy link
@dadlerj

dadlerj Aug 13, 2019

Member

Since this file is in the /shared directory, can it import from /web? I'm afraid this would break in the browser extension (and another important question: do we want this to be logged every time this class loads ini the browser extension as well)?

@felixfbecker
Copy link
Member

left a comment

I'll leave to @dadlerj to judge whether this is the right approach to log these. No other comments on the code that is not reported by ESLint already

// TODO: Send these logs to the backend to anonymously log active extensions from private instances.
extensionRegistry.activeExtensions.subscribe(extensions => {
const activeExtensions = extensions.map(activeExtension => activeExtension.id)
if (telemetryService) {

This comment has been minimized.

Copy link
@felixfbecker

felixfbecker Aug 14, 2019

Member

This if should better be outside of the subscribe() so all the other code doesn't need to run if it is undefined

@@ -146,6 +146,7 @@ export function createPlatformContext(
sideloadedExtensionURL: isInPage
? new LocalStorageSubject<string | null>('sideloadedExtensionURL', null)
: new ExtensionStorageSubject('sideloadedExtensionURL', null),
telemetryService: undefined,

This comment has been minimized.

Copy link
@felixfbecker

felixfbecker Aug 14, 2019

Member

This is optional so doesn't need to be supplied

@@ -51,7 +52,7 @@ import { UserSettingsSidebarItems } from './user/settings/UserSettingsSidebar'

import './SourcegraphWebApp.scss'

export interface SourcegraphWebAppProps extends KeybindingsProps {
export interface SourcegraphWebAppProps extends KeybindingsProps, TelemetryProps {

This comment has been minimized.

Copy link
@felixfbecker

felixfbecker Aug 14, 2019

Member

This doesn't seem to be used?

This comment has been minimized.

Copy link
@vanesa

vanesa Aug 14, 2019

Author Member

oops, yes thanks, removed.

@vanesa vanesa force-pushed the vo/track-extensions-usage branch from 60cbd38 to 554805f Aug 14, 2019

// Anonymously log which external extensions are active on sourcegraph.com.
// TODO: Send these logs to the backend to anonymously log active extensions from private instances.
if (telemetryService) {
extensionRegistry.activeExtensions.subscribe(extensions => {

This comment has been minimized.

Copy link
@felixfbecker

felixfbecker Aug 14, 2019

Member

Please don't forget to fix this ESLint warning (Subscription should not be ignored, see code below this for an example)

This comment has been minimized.

Copy link
@felixfbecker
extensionRegistry.activeExtensions.subscribe(extensions => {
const activeExtensions = extensions.map(activeExtension => activeExtension.id)
for (const extension of activeExtensions) {
telemetryService.log(extension)

This comment has been minimized.

Copy link
@felixfbecker

felixfbecker Aug 14, 2019

Member

@dadlerj I thought the argument to log() is an event name - do we want arbitrary extension names to be event names? Or should it be an argument instead?

This comment has been minimized.

Copy link
@vanesa

vanesa Aug 14, 2019

Author Member

I'll be updating telemetryService as this isn't safe.

This comment has been minimized.

Copy link
@dadlerj

dadlerj Aug 14, 2019

Member

Agreed — just discussed with @vanesa. I think adding the second argument to the TelemetryService interface is the correct approach here.

@vanesa vanesa marked this pull request as ready for review Aug 14, 2019

@vanesa vanesa requested a review from lguychard as a code owner Aug 14, 2019

@vanesa vanesa requested a review from dadlerj Aug 14, 2019

@codecov

This comment has been minimized.

Copy link

commented Aug 14, 2019

Codecov Report

Merging #5177 into master will increase coverage by <.01%.
The diff coverage is 42.85%.

@@            Coverage Diff             @@
##           master    #5177      +/-   ##
==========================================
+ Coverage   45.77%   45.78%   +<.01%     
==========================================
  Files         736      736              
  Lines       45416    45412       -4     
  Branches     2621     2622       +1     
==========================================
+ Hits        20791    20792       +1     
+ Misses      22614    22609       -5     
  Partials     2011     2011
Impacted Files Coverage Δ
shared/src/api/client/services.ts 100% <ø> (ø) ⬆️
shared/src/telemetry/telemetryService.ts 100% <ø> (ø) ⬆️
browser/src/shared/tracking/eventLogger.tsx 2.7% <0%> (ø) ⬆️
shared/src/api/client/connection.ts 88.57% <100%> (ø) ⬆️
shared/src/api/client/api/extensions.ts 44% <40%> (-3.62%) ⬇️
cmd/frontend/internal/httpapi/internal.go 6.1% <0%> (+0.02%) ⬆️
cmd/frontend/graphqlbackend/search_symbols.go 10.78% <0%> (+0.07%) ⬆️
pkg/conf/computed.go 26.2% <0%> (+0.87%) ⬆️

@vanesa vanesa force-pushed the vo/track-extensions-usage branch from e842d80 to ddb8742 Aug 15, 2019

@vanesa vanesa requested a review from felixfbecker Aug 19, 2019

@vanesa vanesa requested review from ryan-blunden and sqs as code owners Aug 19, 2019

@vanesa vanesa force-pushed the vo/track-extensions-usage branch from 6e46b2b to 6657b30 Aug 19, 2019

@vanesa vanesa removed the request for review from sqs Aug 19, 2019

@vanesa vanesa removed request for ryan-blunden and dadlerj Aug 19, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.