Report Connect usage events by gzdunek · Pull Request #19564 · gravitational/teleport

gzdunek · 2022-12-21T12:44:57Z

Part of #19000
Requires #19378 to be merged first. ✅
Requires #19774 to be merged first (see #19564 (comment)) ✅

This PR adds actual events sending mechanism, it receives events from Electron, ~~converts them to prehog model~~ anonyzmies them and then sends in batches.

EDIT:
In the first attempt I had separate definitions for prehog events in tshd. It was ineffective approach and resulted in quite a lot of boilerplate (maintaining two, very similar protos and then having to convert one model to another). After a discussion with Rafał, we decided to use prehog protos directly in tshd protos.

gzdunek · 2022-12-21T18:01:53Z

My thoughts about this PR:
Anonymizing and converting tshd events to prehog events introduces quite a lot of boilerplate (the events have almost the same properties and I need manually convert each of them). I’m considering getting rid of tshd's usage_events.proto and instead operating only prehog's events.
It would require:

Performing anonymization before event leaves electron - shouldn’t be difficult, I can add a gRPC call that will run our anonymizer against the provided value
Importing prehog's connect.proto to teleterm's service.proto - for some reason it doesn’t work, I get an error saying that imported proto doesn't exist
Generating JS files for connect.proto

What do you think about it? Do you like the current solution, where we send authClusterId with every event that needs to be anonymized? I'm afraid it can be confusing, as we create almost complete events in Electron and then we perform one more thing in tshd :/

avatus

I only had a chance to look at the types today. Will finish my review tomorrow. Super neat so far tho!

avatus · 2022-12-21T23:28:33Z

+  }
+}
+
+message SubmitConnectEventResponse {}


We can reuse our EmptyResponse for this in service.proto instead of creating a new empty response here. Like this here

We could use google.protobuf.Empty here, as long as it doesn't return linter errors (I'm not sure about this).

Historically, I haven't been consistent with it inside lib/teleterm. tshd_events_service.proto explicitly defines custom empty messages in order to keep backwards compatibility.

But I did a quick search right now and it turns out reusing google.protobuf.Empty (or something similar) is not a problem for backwards compatibility. Eric Anderson from the gRPC team says that old clients will not have a problem with this and the only thing it breaks is the generated types.

So I think going forward we could use google.protobuf.Empty, as we already do in the project. Unless the linter is now set up to require you to define a custom message for each RPC.

The .proto is imported from gravitational/prehog, which enforces the default Buf lint rules RPC_REQUEST_STANDARD_NAME and RPC_RESPONSE_STANDARD_NAME.

avatus · 2022-12-22T00:13:25Z

+  ConnectUsageEventOneOf event = 3;
+}
+
+message EventReportedResponse {}


This can also be an EmptyResponse

Done, I think it's ok to use our common EmptyResponse instead of google.protobuf.Empty.

ravicious · 2022-12-22T11:08:04Z

+			// Note: this results in retrying the entire batch, which probably
+			// isn't ideal.
+			req := connect.NewRequest(event.Event)
+			if _, err := client.SubmitConnectEvent(ctx, req); err != nil {
+				errors = append(errors, err)
+			}


Do we need to address this before merging?

No, it works this way for Teleport events too, we can fix it later.

ravicious · 2022-12-22T12:49:38Z

+
+option go_package = "github.com/gravitational/teleport/lib/teleterm/v1";
+
+message ReportEventRequest {


I’m considering getting rid of tshd's usage_events.proto and instead operating only prehog's events. It would require:

Performing anonymization before event leaves electron - shouldn’t be difficult, I can add a gRPC call that will run our anonymizer against the provided value

Importing prehog's connect.proto to teleterm's service.proto - for some reason it doesn’t work, I get an error saying that imported proto doesn't exist

Generating JS files for connect.proto

What do you think about it? Do you like the current solution, where we send authClusterId with every event that needs to be anonymized? I'm afraid it can be confusing, as we create almost complete events in Electron and then we perform one more thing in tshd :/

For me it's not confusing as much as it's just a lot of boilerplate.

From the side of the Electron app, I think the API looks pretty much alright – the Electron app should just send events and I don't think it wants to care about anonymization, especially since it's unable to anonymize anything on its own.

Could the Electron app send prehog's events to tshd through TerminalService and then tshd would anonymize the required fields before passing the events further?

If that would be possible, theoretically we could then do the minimal amount of work possible instead of having to rewrap every event.

As for importing connect.proto to service.proto:

The protocol compiler searches for imported files in a set of directories specified on the protocol compiler command line using the -I/--proto_path flag. If no flag was given, it looks in the directory in which the compiler was invoked. In general you should set the --proto_path flag to the root of your project and use fully qualified names for all imports.

https://developers.google.com/protocol-buffers/docs/proto3#importing_definitions

AFAIK we now use buf instead of protoc, so maybe enabling that requires changes to buf's config.

Could the Electron app send prehog's events to tshd through TerminalService and then tshd would anonymize the required fields before passing the events further?

Unfortunately, it is not possible as tshd events (the ones that need to be anonymized) have one additional field - auth_cluster_id. It is needed to create the anonymizer. The prehog events don't have this field.

EDIT: we can send event + auth cluster id

I just pushed the changes, so we don't have tshd usage events anymore and there is much less boilerplate.

We should merge changes to the protobuf generation first, @ravicious promised to create a PR for that :)

# Conflicts: # e

ravicious · 2023-01-02T15:49:09Z

I opened #19774 which reorganizes protos into a single workspace.

I made a couple of small changes compared to #19700. Once #19774 is merged, you will need to do the following in this PR:

Add lib/prehog/buf-teleterm.gen.yaml

lib/prehog/buf-teleterm.gen.yaml

# buf-teleterm.gen.yaml is identical to buf.gen.yaml,
# with the exception of three addidiotal JS plugins.
version: v1
managed:
  enabled: true
  go_package_prefix:
    default: github.com/gravitational/teleport/lib/prehog/gen
plugins:
  - name: go
    path: lib/prehog/bin/protoc-gen-go
    out: lib/prehog/gen
    opt:
      - paths=source_relative
  - name: connect-go
    path: lib/prehog/bin/protoc-gen-connect-go
    out: lib/prehog/gen
    opt:
      - paths=source_relative

  - name: js
    out: lib/prehog/gen-js
    opt:
      - import_style=commonjs,binary
  - name: grpc
    out: lib/prehog/gen-js
    opt: grpc_js
    path: grpc_tools_node_protoc_plugin
  - name: ts
    out: lib/prehog/gen-js
    opt: "service=grpc-node"

Extend grpc-teleterm/host to generate prehog protos as well.

grpc-teleterm/host: protos/all
	$(BUF) generate --template=lib/prehog/buf-teleterm.gen.yaml lib/prehog/proto
	$(BUF) generate --template=lib/teleterm/buf.gen.yaml lib/teleterm/api/proto

Add go_package in lib/prehog/proto/prehog/v1alpha/teleport.proto

option go_package = "github.com/gravitational/teleport/lib/prehog/gen/prehog/v1alpha";

# Conflicts: # e

… context

gzdunek · 2023-01-03T16:45:05Z

License check was failing on the following files:

lib/prehog/gen-js/prehog/v1alpha/teleport_grpc_pb.d.ts
lib/prehog/gen-js/prehog/v1alpha/connect_grpc_pb.d.ts
lib/prehog/gen-js/prehog/v1alpha/connect_pb.d.ts
lib/prehog/gen-js/prehog/v1alpha/teleport_pb.d.ts

I added lib/prehog/gen-js/** to the ignored files list.

…g it

gzdunek · 2023-01-10T11:33:04Z

I updated the PR:

updated connect.proto to match the final version from prehog
added changes related to JS files generation (from Report Connect usage events #19564 (comment))
removed hardcoded prehog address, instead it will be passed from Electron (similarly to other runtime values)

ravicious · 2023-01-10T13:37:32Z

+  string cluster_name = 1;
+  // anonymized
+  string user_name = 2;
+  bool is_upload = 3;


What's the advantage of doing it this way vs using a string with "upload"/"download"?

I followed Edoardo suggestion, I think isUpload is easier to understand than direction.

ravicious · 2023-01-10T13:45:08Z

Let's put it below DaemonCertsDir and above DatabaseService.

Right, I meant to put it like that.

gzdunek added 4 commits December 21, 2022 10:17

Make UsageReporter reusable, move it to the new package

e3c6feb

Add TeleportUsageReporter

0a2d5b7

Rename UsageSubmitFunc and SubmitAnonymizedUsageEvents

ff71bb7

Rename NewUsageReporterTeleport to NewTeleportUsageReporter

3325ff8

This was referenced Dec 21, 2022

Make UsageReporter reusable #19378

Merged

Connect: Collect usage events gravitational/webapps#1451

Merged

gzdunek requested review from avatus and ravicious December 21, 2022 18:02

avatus reviewed Dec 22, 2022

View reviewed changes

ravicious reviewed Dec 22, 2022

View reviewed changes

gzdunek added 11 commits December 29, 2022 15:55

Merge branch 'master' into gzdunek/make-usage-reporter-reusable

aaa43c8

Remove timestamp from test

4c1febe

Update e-ref

474f608

Fix imports

9cbcc4a

Rename receivers

7273198

Add comment

8a35ae7

Merge branch 'master' into gzdunek/make-usage-reporter-reusable

c94e8f4

# Conflicts: # e

Bring back original MaxBatchAge in tests

04be558

Report Connect usage events

e6f2fb8

Fix comment for retryAttempts

909bacf

Remove tshd usage events, opearate only on prehog protos

00769c1

gzdunek force-pushed the gzdunek/add-usage-reporter-to-connect branch from f140eff to 00769c1 Compare December 30, 2022 15:56

Base automatically changed from gzdunek/make-usage-reporter-reusable to master December 30, 2022 17:24

gzdunek added 5 commits January 3, 2023 16:01

Change prehog_event to prehog_req

a823e30

Merge branch 'master' into gzdunek/add-usage-reporter-to-connect

0535122

# Conflicts: # e

Populate failed events array

d6fdd20

Rename things a little

ffde8ca

Fix lint issues

de1816c

Rename ReportEvent to ReportUsageEvent in gRPC service for better…

54a12ce

… context

gzdunek marked this pull request as ready for review January 3, 2023 16:57

github-actions Bot added the size/md label Jan 3, 2023

github-actions Bot requested review from hatched and nklaassen January 3, 2023 16:57

gzdunek added 2 commits January 3, 2023 18:06

Do not check license in generated prehog js files

f24135a

Update protos

c7b59a0

gzdunek force-pushed the gzdunek/add-usage-reporter-to-connect branch from 805c4ec to c7b59a0 Compare January 3, 2023 17:06

Merge branch 'master' into gzdunek/add-usage-reporter-to-connect

9d85079

avatus approved these changes Jan 5, 2023

View reviewed changes

Merge branch 'master' into gzdunek/add-usage-reporter-to-connect

c25f258

nklaassen approved these changes Jan 10, 2023

View reviewed changes

github-actions Bot removed the request for review from hatched January 10, 2023 00:50

gzdunek added 4 commits January 10, 2023 11:57

Merge branch 'master' into gzdunek/add-usage-reporter-to-connect

80cee73

Update Connect proto files to match prehog

9e971ed

Generate JS files for prehog protos

1906460

Pass prehog address as argument to tshd process instead of hard-codin…

5466f3c

…g it

ravicious self-requested a review January 10, 2023 11:56

ravicious reviewed Jan 10, 2023

View reviewed changes

gzdunek added 2 commits January 10, 2023 14:59

Keep DaemonPrehogAddr with other prehog values

6bcfaeb

Merge branch 'master' into gzdunek/add-usage-reporter-to-connect

464588d

ravicious approved these changes Jan 10, 2023

View reviewed changes

gzdunek added 2 commits January 10, 2023 17:34

Merge branch 'master' into gzdunek/add-usage-reporter-to-connect

012201f

Merge branch 'master' into gzdunek/add-usage-reporter-to-connect

e45fb3b

gzdunek merged commit 584e96e into master Jan 10, 2023

timothyb89 mentioned this pull request Feb 3, 2023

[v11] Add usage events for certificate issuance and additional session types (#20662) #21192

Merged

gzdunek deleted the gzdunek/add-usage-reporter-to-connect branch March 20, 2023 16:30


		option go_package = "github.com/gravitational/teleport/lib/teleterm/v1";

		message ReportEventRequest {

Conversation

gzdunek commented Dec 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gzdunek commented Dec 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

avatus left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gzdunek Dec 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ravicious commented Jan 2, 2023 • edited by gzdunek Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gzdunek commented Jan 3, 2023

Uh oh!

gzdunek commented Jan 10, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

gzdunek commented Dec 21, 2022 •

edited

Loading

gzdunek commented Dec 21, 2022 •

edited

Loading

gzdunek Dec 22, 2022 •

edited

Loading

ravicious commented Jan 2, 2023 •

edited by gzdunek

Loading