Add usage reporting pkg #396
Conversation
Pull Request Test Coverage Report for Build 5489015002
💛 - Coveralls |
|
/gcbrun |
usage/usage.go
Outdated
| // limitations under the License. | ||
|
|
||
| // Package usage handles anonymous usage reporting. | ||
| package usage |
There was a problem hiding this comment.
I am not sure usage is the best name for a package. Normally usage is related to the how to use the command. Maybe metrics or something similar is a better name?
usage/usage.go
Outdated
| log "k8s.io/klog/v2" | ||
| ) | ||
|
|
||
| const ( |
There was a problem hiding this comment.
I am not too fond of these being consts. They will need to be configurable at some point.
usage/usage.go
Outdated
|
|
||
| // Reporter is a client that reports KNE usage events using PubSub. | ||
| type Reporter struct { | ||
| pubsubClient *pubsub.Client |
There was a problem hiding this comment.
Is there a reason you didn't just embed pubsub.Client? If you do embed you can get rid of Reporter.Close.
There was a problem hiding this comment.
now that topic is also a struct field, Close correctly closes/stops both client and topic
usage/usage.go
Outdated
| if err != nil { | ||
| return err | ||
| } | ||
| t := r.pubsubClient.Topic(topicID) |
There was a problem hiding this comment.
Is there any value in adding a topic field to Reporter and then initialize it in NewReporter?
usage/usage.go
Outdated
| } | ||
| t := r.pubsubClient.Topic(topicID) | ||
| res := t.Publish(ctx, &pubsub.Message{Data: msg}) | ||
| id, err := res.Get(ctx) |
There was a problem hiding this comment.
Since res is only used here you can combine these two lines.
usage/usage.go
Outdated
| } | ||
|
|
||
| // ReportDeployClusterStart reports that a cluster deployment has started. The UUID of the | ||
| // reported event is returned along with an error if the event reporting failed. |
There was a problem hiding this comment.
The comment does not match the code. The return is UUID xor Error. I think you meant to say if an error is returned the error will contain the UUID (the UUID is not returned directly).
usage/usage.go
Outdated
| } | ||
|
|
||
| // ReportCreateTopologyStart reports that a topology creation has started. The UUID of the | ||
| // reported event is returned along with an error if the event reporting failed. |
usage/usage.go
Outdated
| return event.GetUuid(), nil | ||
| } | ||
|
|
||
| // ReportCreateTopologyEnd reports that a topology creation has ended. |
There was a problem hiding this comment.
nit: unlike above you don't include the comment about errors in this description.
usage/usage.go
Outdated
|
|
||
| func newEvent() *epb.KNEEvent { | ||
| return &epb.KNEEvent{ | ||
| Uuid: uuid.New(), |
There was a problem hiding this comment.
Perhaps the id should be passed in rather than creating a UUID here. ReportDeployClusterStart can then call newEvent(uuid.New()) and make it clear it is creating a new UUID rather than using an existing one.
usage/usage.go
Outdated
|
|
||
| func newEvent() *epb.KNEEvent { | ||
| return &epb.KNEEvent{ | ||
| Uuid: uuid.New(), |
There was a problem hiding this comment.
nit: UUID is an acronym so it really should be UUID or uuid and not Uuid.
There was a problem hiding this comment.
Agreed, but this is generated proto code
|
/gcbrun |
No description provided.