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

fix: don't pass context to segment telemetry #138

Closed
wants to merge 1 commit into from
Closed

fix: don't pass context to segment telemetry #138

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion internal/cmd/local/local/install.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -182,7 +182,7 @@ func (c *Command) Install(ctx context.Context, opts *InstallOpts) error {
span.SetAttributes(attribute.Bool("migrate", opts.Migrate))
if opts.Migrate {
c.spinner.UpdateText("Migrating airbyte data")
if err := c.tel.Wrap(ctx, telemetry.Migrate, func() error { return migrate.FromDockerVolume(ctx, c.docker.Client, "airbyte_db") }); err != nil {
if err := c.tel.Wrap(telemetry.Migrate, func() error { return migrate.FromDockerVolume(ctx, c.docker.Client, "airbyte_db") }); err != nil {
pterm.Error.Println("Failed to migrate data from previous Airbyte installation")
return fmt.Errorf("unable to migrate data from previous airbyte installation: %w", err)
}
Expand Down
2 changes: 1 addition & 1 deletion internal/cmd/local/local_credentials.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ func (cc *CredentialsCmd) Run(ctx context.Context, provider k8s.Provider, telCli

spinner := &pterm.DefaultSpinner

return telClient.Wrap(ctx, telemetry.Credentials, func() error {
return telClient.Wrap(telemetry.Credentials, func() error {
k8sClient, err := defaultK8s(provider.Kubeconfig, provider.Context)
if err != nil {
pterm.Error.Println("No existing cluster found")
Expand Down
2 changes: 1 addition & 1 deletion internal/cmd/local/local_deployments.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ func (d *DeploymentsCmd) Run(ctx context.Context, telClient telemetry.Client, k8
return err
}

return telClient.Wrap(ctx, telemetry.Deployments, func() error {
return telClient.Wrap(telemetry.Deployments, func() error {
return d.deployments(ctx, k8sClient, spinner)
})
}
Expand Down
2 changes: 1 addition & 1 deletion internal/cmd/local/local_install.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,7 @@ func (i *InstallCmd) Run(ctx context.Context, provider k8s.Provider, telClient t
return err
}

return telClient.Wrap(ctx, telemetry.Install, func() error {
return telClient.Wrap(telemetry.Install, func() error {
spinner.UpdateText(fmt.Sprintf("Checking for existing Kubernetes cluster '%s'", provider.ClusterName))

cluster, err := provider.Cluster(ctx)
Expand Down
2 changes: 1 addition & 1 deletion internal/cmd/local/local_status.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ func (s *StatusCmd) Run(ctx context.Context, provider k8s.Provider, telClient te
return err
}

return telClient.Wrap(ctx, telemetry.Status, func() error {
return telClient.Wrap(telemetry.Status, func() error {
return status(ctx, provider, telClient, spinner)
})
}
Expand Down
2 changes: 1 addition & 1 deletion internal/cmd/local/local_uninstall.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ func (u *UninstallCmd) Run(ctx context.Context, provider k8s.Provider, telClient
return fmt.Errorf("unable to determine docker installation status: %w", err)
}

return telClient.Wrap(ctx, telemetry.Uninstall, func() error {
return telClient.Wrap(telemetry.Uninstall, func() error {
spinner.UpdateText(fmt.Sprintf("Checking for existing Kubernetes cluster '%s'", provider.ClusterName))

cluster, err := provider.Cluster(ctx)
Expand Down
9 changes: 4 additions & 5 deletions internal/telemetry/client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
package telemetry

import (
"context"
"errors"
"fmt"
"net/http"
Expand Down Expand Up @@ -34,19 +33,19 @@ const (
// Client interface for telemetry data.
type Client interface {
// Start should be called as quickly as possible.
Start(context.Context, EventType) error
Start(EventType) error
// Success should be called only if the activity succeeded.
Success(context.Context, EventType) error
Success(EventType) error
// Failure should be called only if the activity failed.
Failure(context.Context, EventType, error) error
Failure(EventType, error) error
// Attr should be called to add additional attributes to this activity.
Attr(key, val string)
// User returns the user identifier being used by this client
User() string
// Wrap wraps the func() error with the EventType,
// calling the Start, Failure or Success methods correctly based on
// the behavior of the func() error
Wrap(context.Context, EventType, func() error) error
Wrap(EventType, func() error) error
}

type getConfig struct {
Expand Down
26 changes: 12 additions & 14 deletions internal/telemetry/mock.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,27 +1,25 @@
package telemetry

import "context"

var _ Client = (*MockClient)(nil)

type MockClient struct {
attrs map[string]string
start func(context.Context, EventType) error
success func(context.Context, EventType) error
failure func(context.Context, EventType, error) error
wrap func(context.Context, EventType, func() error) error
start func(EventType) error
success func(EventType) error
failure func(EventType, error) error
wrap func(EventType, func() error) error
}

func (m *MockClient) Start(ctx context.Context, eventType EventType) error {
return m.start(ctx, eventType)
func (m *MockClient) Start(eventType EventType) error {
return m.start(eventType)
}

func (m *MockClient) Success(ctx context.Context, eventType EventType) error {
return m.success(ctx, eventType)
func (m *MockClient) Success(eventType EventType) error {
return m.success(eventType)
}

func (m *MockClient) Failure(ctx context.Context, eventType EventType, err error) error {
return m.failure(ctx, eventType, err)
func (m *MockClient) Failure(eventType EventType, err error) error {
return m.failure(eventType, err)
}

func (m *MockClient) Attr(key, val string) {
Expand All @@ -35,6 +33,6 @@ func (m *MockClient) User() string {
return "test-user"
}

func (m *MockClient) Wrap(ctx context.Context, et EventType, f func() error) error {
return m.wrap(ctx, et, f)
func (m *MockClient) Wrap(et EventType, f func() error) error {
return m.wrap(et, f)
}
12 changes: 4 additions & 8 deletions internal/telemetry/noop.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,24 +1,20 @@
package telemetry

import (
"context"
)

var _ Client = (*NoopClient)(nil)

// NoopClient client, all methods are no-ops.
type NoopClient struct {
}

func (n NoopClient) Start(context.Context, EventType) error {
func (n NoopClient) Start(EventType) error {
return nil
}

func (n NoopClient) Success(context.Context, EventType) error {
func (n NoopClient) Success(EventType) error {
return nil
}

func (n NoopClient) Failure(context.Context, EventType, error) error {
func (n NoopClient) Failure(EventType, error) error {
return nil
}

Expand All @@ -28,6 +24,6 @@ func (n NoopClient) User() string {
return ""
}

func (n NoopClient) Wrap(ctx context.Context, et EventType, f func() error) error {
func (n NoopClient) Wrap(et EventType, f func() error) error {
return f()
}
12 changes: 5 additions & 7 deletions internal/telemetry/noop_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
package telemetry

import (
"context"
"errors"
"testing"

Expand All @@ -11,14 +10,13 @@ import (

func TestNoopClient(t *testing.T) {
cli := NoopClient{}
ctx := context.Background()
if err := cli.Start(ctx, Install); err != nil {
if err := cli.Start(Install); err != nil {
t.Error(err)
}
if err := cli.Success(ctx, Install); err != nil {
if err := cli.Success(Install); err != nil {
t.Error(err)
}
if err := cli.Failure(ctx, Install, errors.New("")); err != nil {
if err := cli.Failure(Install, errors.New("")); err != nil {
t.Error(err)
}

Expand All @@ -40,7 +38,7 @@ func TestNoopClient_Wrap(t *testing.T) {

cli := NoopClient{}

if err := cli.Wrap(context.Background(), Install, fn); err != nil {
if err := cli.Wrap(Install, fn); err != nil {
t.Fatal("unexpected error", err)
}

Expand All @@ -59,7 +57,7 @@ func TestNoopClient_Wrap(t *testing.T) {

cli := NoopClient{}

err := cli.Wrap(context.Background(), Install, fn)
err := cli.Wrap(Install, fn)
if d := cmp.Diff(expectedErr, err, cmpopts.EquateErrors()); d != "" {
t.Errorf("function should have returned an error (-want, +got): %s", d)
}
Expand Down
25 changes: 12 additions & 13 deletions internal/telemetry/segment.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ package telemetry

import (
"bytes"
"context"
"fmt"
"maps"
"net/http"
Expand Down Expand Up @@ -66,16 +65,16 @@ func NewSegmentClient(cfg Config, opts ...Option) *SegmentClient {
return cli
}

func (s *SegmentClient) Start(ctx context.Context, et EventType) error {
return s.send(ctx, Start, et, nil)
func (s *SegmentClient) Start(et EventType) error {
return s.send(Start, et, nil)
}

func (s *SegmentClient) Success(ctx context.Context, et EventType) error {
return s.send(ctx, Success, et, nil)
func (s *SegmentClient) Success(et EventType) error {
return s.send(Success, et, nil)
}

func (s *SegmentClient) Failure(ctx context.Context, et EventType, err error) error {
return s.send(ctx, Failed, et, err)
func (s *SegmentClient) Failure(et EventType, err error) error {
return s.send(Failed, et, err)
}

func (s *SegmentClient) Attr(key, val string) {
Expand All @@ -86,17 +85,17 @@ func (s *SegmentClient) User() string {
return s.cfg.AnalyticsID.toUUID().String()
}

func (s *SegmentClient) Wrap(ctx context.Context, et EventType, f func() error) error {
func (s *SegmentClient) Wrap(et EventType, f func() error) error {
attemptSuccessFailure := true

if err := s.Start(ctx, et); err != nil {
if err := s.Start(et); err != nil {
pterm.Debug.Printfln("Unable to send telemetry start data: %s", err)
attemptSuccessFailure = false
}

if err := f(); err != nil {
if attemptSuccessFailure {
if errTel := s.Failure(ctx, et, err); errTel != nil {
if errTel := s.Failure(et, err); errTel != nil {
pterm.Debug.Printfln("Unable to send telemetry failure data: %s", errTel)
}
}
Expand All @@ -105,7 +104,7 @@ func (s *SegmentClient) Wrap(ctx context.Context, et EventType, f func() error)
}

if attemptSuccessFailure {
if err := s.Success(ctx, et); err != nil {
if err := s.Success(et); err != nil {
pterm.Debug.Printfln("Unable to send telemetry success data: %s", err)
}
}
Expand All @@ -118,7 +117,7 @@ const (
url = "https://api.segment.io/v1/track"
)

func (s *SegmentClient) send(ctx context.Context, es EventState, et EventType, ee error) error {
func (s *SegmentClient) send(es EventState, et EventType, ee error) error {
properties := map[string]string{
"deployment_method": "abctl",
"session_id": s.sessionID.String(),
Expand Down Expand Up @@ -150,7 +149,7 @@ func (s *SegmentClient) send(ctx context.Context, es EventState, et EventType, e
return fmt.Errorf("unable to create request body: %w", err)
}

req, err := http.NewRequestWithContext(ctx, http.MethodPost, url, bytes.NewBuffer(data))
req, err := http.NewRequest(http.MethodPost, url, bytes.NewBuffer(data))
Copy link
Member

Choose a reason for hiding this comment

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

What if we only made this change, but still passed the context through everything?

Copy link
Member

Choose a reason for hiding this comment

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

I'm thinking about this in a similar way that the go slog package handles cancelled contexts, in that they ignore them in certain cases; e.g.

The signature of Handle is
Handle(context.Context, Record) error

The context is provided to support applications that provide logging information along the call chain. In a break with usual Go practice, the Handle method should not treat a canceled context as a signal to stop work.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure, that's a good idea.

if err != nil {
return fmt.Errorf("unable to create request: %w", err)
}
Expand Down
Loading