feat(posthog): Add Telemetry client Posthog #781
Conversation
javirln
changed the title
|
Secrets on the repository updated already to the proper project. |
I've moved the message to the |
There was a problem hiding this comment.
Thanks,
I tried to figure out the flag issue and boy it's quite confusing, ..., I kinda managed to fix it but I do not like my solution. In am ok going with your solution for now but we should add a task to fix it.
Re telemetry performance, I'll take a quick look and see if I find anything, once done I think I'll be ready to approve this and move forward. Thanks!
app/cli/main.go
Outdated
| os.Exit(exitCode) | ||
| } | ||
| // Measure the execution time | ||
| executionTime := time.Since(timeStart) |
There was a problem hiding this comment.
do we need recording the duration? If we do not, I'd not add it yet. For these kind of nice to have features I tend to follow YAGNI pretty aggressively.
|
Refactored all code with:
|
app/cli/main.go
Outdated
| if err := rootCmd.Execute(); err != nil { | ||
| var msg string |
There was a problem hiding this comment.
we can probably revert all the changes in this file. For example, having the msg var doesn't seem to make sense anymore.
| } | ||
|
|
||
| // DetermineUserID Determines the user ID using available information or generates a random one. | ||
| func DetermineUserID(tags Tags) string { |
There was a problem hiding this comment.
DetermineUserID can be downcase if you move the tests to the same package
There was a problem hiding this comment.
I've added it into a separated packaged because if added to the same package it creates an import cycle between the mocks and the implementation:
FAIL github.com/chainloop-dev/chainloop/app/cli/internal/telemetry [setup failed]
package github.com/chainloop-dev/chainloop/app/cli/internal/telemetry
imports github.com/chainloop-dev/chainloop/app/cli/internal/telemetry/mocks
imports github.com/chainloop-dev/chainloop/app/cli/internal/telemetry: import cycle not allowed in test
There was a problem hiding this comment.
You are right, this is the problem of using the mocks generated by mockery :(
A workaround is to create another file, telemerty_unit_test.go or similar and have these unit tests.
In general when I end up writing code in a specific way just so it works in the tests, I realize that I might be doing smth wrong and re-evaluate. But up to you.
Thansk!
There was a problem hiding this comment.
A workaround is to create another file, telemerty_unit_test.go or similar and have these unit tests.
But the problem would still be the same, the method will still be public right?
I mean for me, I wouldn't even create a test for this specific function since they are covered by the tests on the Track method that includes it.
| @@ -239,46 +268,125 @@ func loadControlplaneAuthToken(cmd *cobra.Command) (string, error) { | |||
| // 3. API token | |||
| // Each one of them have an associated audience claim that we use to identify the type of token. If the token is not | |||
| // present, nor we cannot match it with one of the expected audience, return nil. | |||
| func parseToken(token string) (*ParsedToken, error) { | |||
| func parseToken(token string) (*parsedToken, error) { | |||
There was a problem hiding this comment.
Not for this PR, but most probably we can have a common interface. Something like
type Token interface {
func Parse(token string) (*parsedToken, error)
}
And then moving the "extracting" logic to their own implementations.
Signed-off-by: Javier Rodriguez <javier@chainloop.dev>
This PR creates a new implementation of a Tracker client based on Posthog.
Users can deactivate telemetry by setting the env variable
DO_NOT_TRACK=1, it follows the convention at: https://consoledonottrack.com/Information being tracked:
Refs: #780