Conversation
gzdunek
force-pushed
the
gzdunek/collect-usage-events
branch
from
eec7d51 to
3a682f9
Compare
|
|
||
| captureProtocolRun( | ||
| uri: ClusterOrResourceUri, | ||
| protocol: 'ssh' | 'kube' | 'db' |
There was a problem hiding this comment.
In the RFD I was planning proxy_db, but I think we can just send db, we will match other teleport events.
ravicious
left a comment
ravicious
left a comment
There was a problem hiding this comment.
I left some questions/comments on the details of the implementations, I'll leave a comment about the general approach soon – I'm not sure if here or under the teleport PR though.
gzdunek
force-pushed
the
gzdunek/collect-usage-events
branch
from
2a85249 to
1e4505e
Compare
|
I incorporated the changes from #1464 |
| return; | ||
| } | ||
|
|
||
| return this.tshClient.reportUsageEvent({ |
There was a problem hiding this comment.
At the moment, every place that calls UsageService does so without checking if reportEvent returns an error.
In the dev version this will show that unhandled rejection modal, in the prod version there will be a log entry in the renderer log but otherwise it'll be silently ignored.
Also, in all cases at the moment the successful usage of a given feature doesn't hinge on successfully reporting the usage, that is when I'm connecting to a server, I don't really care if the usage is reported or not because it's not needed in order to connect to a server.
Any thoughts on how we should approach this?
I was thinking of sending warnings when this happens, OTOH this is not something the user can do anything about. Like imagine Teleport's reporting APIs fail but otherwise the cluster that the user is logged to works just fine.
What if we just logged the error and then ignored it, making it so that UsageService methods don't fail on the call to tshClient?
There was a problem hiding this comment.
While writing this I realized that right now the calls to report the event fail only because authClusterId is missing. But it's not like the events are reported during the RPC that creates the event, that's a batched action.
In that case, perhaps sending warnings would be a good solution here? If tshClient.reportUsageEvent fails, it's due to a bug in our codebase and not a problem with our infrastructure.
In that case, I think surfacing it to the user is worthwhile. Otherwise it's possible that we'd never learn about the problem in the first place, we'd simply see a drop in events reported from Connect.
There was a problem hiding this comment.
Good point, I wasn't sure what to do about it, so I left it without handling (so we would be able to at least see it in dev mode). As you said an error is only shown when there is a problem between Electron and tshd, not because prehog is down for example (this problem will be visible in tshd logs only).
Adding a warning looks like the best option to handle this.
| this.logger.warn( | ||
| `Missing cluster data for ${uri}, skipping protocolUse event` | ||
| ); |
There was a problem hiding this comment.
I'd go with a single handler for this in reportEvent instead of repeating that in every event.
There was a problem hiding this comment.
I mean it's fine for now but I'd probably refactor this later down the line.
# Conflicts: # packages/teleterm/src/mainProcess/fixtures/mocks.ts
| // Allows running tsh in insecure mode (development) | ||
| const isInsecure = dev || argv.includes('--insecure'); | ||
|
|
||
| const PREHOG_ADDR = 'https://reporting-staging.teleportinfra.dev'; // TODO(gzdunek): change to prod before going live |
There was a problem hiding this comment.
See, if we had a proper config file for dev & prod, we could use staging in dev and the prod address in the packaged app.
We have to spend a little bit of time on that some time. I'm thinking of adding something like dotenv. Perhaps there are other libs which are better suited for the kind of app we're working on.
There was a problem hiding this comment.
Sure, I'm happy to do it in a proper way. I think for now we can leave it as it is (we don't have prod env for Connect yet) and I will fix it soon.
3 participants
Part of gravitational/teleport#19000
Teleport counterpart gravitational/teleport#19564
This PR adds actual events collection in the UI/services.
TODO (in separate PRs):
authClusterId- without it events can't be anonymized