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.

Sign up for GitHub

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

Jump to bottom

introduce posthog #367

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

introduce posthog #367

wants to merge 4 commits into from
+211 −19

Conversation

zreigz
Copy link
Member

@zreigz zreigz commented Mar 10, 2023

Summary

Introduce posthog for plural bundle install, build, and deploy commands.

client.Enqueue(posthog.Capture{
  DistinctId: "someID",
  Event:      "cli_deploy",
  Properties: map[string]interface{}{
        "clusterName":        "test-cluster",
        "applicationName":    "bootstrap",
        "applicationID":      "123456-abcdef"
        "packageType":         "Helm",
        "packageName":         "bootstrap",
        "packageId":           "123456-abcdef",
        "packageVersion":      "0.1.0",
        "provider":           "AWS",
        "error":              "some error about the helm install failing",
    }
})

Labels

Test Plan

Checklist

  • If required, I have updated the Plural documentation accordingly.
  • I have added tests to cover my changes.
  • I have added a meaningful title and summary to convey the impact of this PR to a user.
  • I have added relevant labels to this PR to help with categorization for release notes.

@zreigz zreigz added the enhancement New feature or request label Mar 10, 2023
@zreigz zreigz force-pushed the posthog branch 2 times, most recently from 011fce7 to 4d51f4e Compare March 10, 2023 14:59
michaeljguarino
michaeljguarino reviewed Mar 11, 2023
View reviewed changes
cmd/plural/deploy.go
@@ -247,7 +250,25 @@ func (p *Plural) deploy(c *cli.Context) error {
fmt.Println("")
}
}

for _, ch := range man.Charts {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should send this info from the server if we want it in posthog, having the cli send it is error prone and could potentially cause failed installs

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can't track the actual commands users execute server side since it doesn't know when somebody runs plural deploy. Having it in the CLI allows us to have a better metric of which commands people run, how often they run, and if they encountered an error when running the command. This can give us a metric of how many successful deploys have happened versus how many errors have occurred. It can also give us a timeline of a user's actions. For example, user deploys but runs into an error for app X, user then runs install to change a config and build for app X to fix that config, user then runs deploy again and it runs successfully.

Each command that a user executes and if it passes or fails is very important data so we can have a better idea of how many command executions pass versus fail and where they fail exactly.

pkg/scaffold/scaffold.go
@@ -120,6 +121,17 @@ func (s *Scaffold) Execute(wk *wkspace.Workspace, force bool) error {
preflight.Sha = ""
}

for _, ch := range wk.Charts {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similarly, this is better to send server-side

pkg/bundle/installer.go Outdated
return err
}

if err := client.InstallRecipe(recipe.Id); err != nil {
posthogProperty.Error = fmt.Errorf("failed install recipe")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we just add this to our existing error logging wrapper?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have moved error handling to tracked validator

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@michaeljguarino michaeljguarino michaeljguarino left review comments

@davidspek davidspek Awaiting requested review from davidspek

At least 1 approving review is required to merge this pull request.

Assignees

@davidspek davidspek

Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@zreigz @michaeljguarino @davidspek