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.

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

refactor(probeservices): use better naming #1628

Merged
merged 8 commits into from
Jun 25, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
4 changes: 3 additions & 1 deletion internal/engine/experiment_integration_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package engine
import (
"context"
"encoding/json"
"errors"
"net/http"
"net/http/httptest"
"os"
Expand All @@ -11,6 +12,7 @@ import (

"github.com/google/go-cmp/cmp"
"github.com/ooni/probe-cli/v3/internal/model"
"github.com/ooni/probe-cli/v3/internal/probeservices"
"github.com/ooni/probe-cli/v3/internal/registry"
)

Expand Down Expand Up @@ -384,7 +386,7 @@ func TestOpenReportNewClientFailure(t *testing.T) {
Type: "antani",
}
err = exp.OpenReportContext(context.Background())
if err.Error() != "probe services: unsupported endpoint type" {
if !errors.Is(err, probeservices.ErrUnsupportedServiceType) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I upgraded this line to use errors.Is which is the idiomatic way of checking after errors.Is was added to Go.

t.Fatal(err)
}
}
Expand Down
4 changes: 2 additions & 2 deletions internal/engine/session.go
Original file line number Diff line number Diff line change
Expand Up @@ -673,8 +673,8 @@ func (s *Session) MaybeLookupBackendsContext(ctx context.Context) error {
if selected == nil {
return ErrAllProbeServicesFailed
}
s.logger.Infof("session: using probe services: %+v", selected.Endpoint)
s.selectedProbeService = &selected.Endpoint
s.logger.Infof("session: using probe services: %+v", selected.Service)
s.selectedProbeService = &selected.Service
s.availableTestHelpers = selected.TestHelpers
return nil
}
Expand Down
2 changes: 1 addition & 1 deletion internal/engine/session_integration_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -483,7 +483,7 @@ func TestNewOrchestraClientProbeServicesNewClientFailure(t *testing.T) {
svc.Type = "antani" // should really not be supported for a long time
}
client, err := sess.newOrchestraClient(context.Background())
if !errors.Is(err, probeservices.ErrUnsupportedEndpoint) {
if !errors.Is(err, probeservices.ErrUnsupportedServiceType) {
t.Fatal("not the error we expected")
}
if client != nil {
Expand Down
23 changes: 12 additions & 11 deletions internal/probeservices/benchselect.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,8 @@ func Default() []model.OOAPIService {
}}
}

// SortEndpoints gives priority to https, then cloudfronted, then onion.
func SortEndpoints(in []model.OOAPIService) (out []model.OOAPIService) {
// SortServices gives priority to https, then cloudfronted, then onion.
func SortServices(in []model.OOAPIService) (out []model.OOAPIService) {
for _, entry := range in {
if entry.Type == "https" {
out = append(out, entry)
Expand All @@ -39,7 +39,7 @@ func SortEndpoints(in []model.OOAPIService) (out []model.OOAPIService) {
return
}

// OnlyHTTPS returns the HTTPS endpoints only.
// OnlyHTTPS returns the HTTPS services only.
func OnlyHTTPS(in []model.OOAPIService) (out []model.OOAPIService) {
for _, entry := range in {
if entry.Type == "https" {
Expand All @@ -49,9 +49,9 @@ func OnlyHTTPS(in []model.OOAPIService) (out []model.OOAPIService) {
return
}

// OnlyFallbacks returns the fallback endpoints only.
// OnlyFallbacks returns the fallback services only.
func OnlyFallbacks(in []model.OOAPIService) (out []model.OOAPIService) {
for _, entry := range SortEndpoints(in) {
for _, entry := range SortServices(in) {
if entry.Type != "https" {
out = append(out, entry)
}
Expand All @@ -67,15 +67,16 @@ type Candidate struct {
// Err indicates whether the service works.
Err error

// Endpoint is the service endpoint.
Endpoint model.OOAPIService
// Service is the service to use.
Service model.OOAPIService

// TestHelpers contains the data returned by the endpoint.
// TestHelpers contains the data returned by the service when
// querying the /api/v1/test-helpers API endpoint.
TestHelpers map[string][]model.OOAPIService
}

func (c *Candidate) try(ctx context.Context, sess Session) {
client, err := NewClient(sess, c.Endpoint)
client, err := NewClient(sess, c.Service)
if err != nil {
c.Err = err
return
Expand All @@ -85,11 +86,11 @@ func (c *Candidate) try(ctx context.Context, sess Session) {
c.Duration = time.Since(start)
c.Err = err
c.TestHelpers = testhelpers
sess.Logger().Debugf("probe services: %+v: %+v %s", c.Endpoint, err, c.Duration)
sess.Logger().Debugf("probe services: %+v: %+v %s", c.Service, err, c.Duration)
}

func try(ctx context.Context, sess Session, svc model.OOAPIService) *Candidate {
candidate := &Candidate{Endpoint: svc}
candidate := &Candidate{Service: svc}
candidate.try(ctx, sess)
return candidate
}
Expand Down
20 changes: 10 additions & 10 deletions internal/probeservices/probeservices.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
// Package probeservices contains code to contact OONI probe services.
//
// The probe services are HTTPS endpoints distributed across a bunch of data
// The probe services are HTTPS services distributed across a bunch of data
// centres implementing a bunch of OONI APIs. When started, OONI will benchmark
// the available probe services and select the fastest one. Eventually all the
// possible OONI APIs will run as probe services.
Expand Down Expand Up @@ -32,8 +32,8 @@ import (
)

var (
// ErrUnsupportedEndpoint indicates that we don't support this endpoint type.
ErrUnsupportedEndpoint = errors.New("probe services: unsupported endpoint type")
// ErrUnsupportedServiceType indicates that we don't support this service type.
ErrUnsupportedServiceType = errors.New("probe services: unsupported service type")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We generally do not change error strings. However, this error string is for a very uncommon case, because the service type should always be supported and so it's fine to modify it for consistency.


// ErrUnsupportedCloudFrontAddress indicates that we don't support this
// cloudfront address (e.g. wrong scheme, presence of port).
Expand Down Expand Up @@ -90,11 +90,11 @@ func (c Client) GetCredsAndAuth() (*model.OOAPILoginCredentials, *model.OOAPILog
return creds, auth, nil
}

// NewClient creates a new client for the specified probe services endpoint. This
// function fails, e.g., we don't support the specified endpoint.
func NewClient(sess Session, endpoint model.OOAPIService) (*Client, error) {
// NewClient creates a new client for the specified probe services service. This
// function fails, e.g., we don't support the specified service.
func NewClient(sess Session, service model.OOAPIService) (*Client, error) {
client := &Client{
BaseURL: endpoint.Address,
BaseURL: service.Address,
HTTPClient: sess.DefaultHTTPClient(),
Host: "",
KVStore: sess.KeyValueStore(),
Expand All @@ -104,7 +104,7 @@ func NewClient(sess Session, endpoint model.OOAPIService) (*Client, error) {
StateFile: NewStateFile(sess.KeyValueStore()),
UserAgent: sess.UserAgent(),
}
switch endpoint.Type {
switch service.Type {
case "https":
return client, nil
case "cloudfront":
Expand All @@ -119,13 +119,13 @@ func NewClient(sess Session, endpoint model.OOAPIService) (*Client, error) {
return nil, ErrUnsupportedCloudFrontAddress
}
client.Host = URL.Hostname()
URL.Host = endpoint.Front
URL.Host = service.Front
client.BaseURL = URL.String()
if _, err := url.Parse(client.BaseURL); err != nil {
return nil, err
}
return client, nil
default:
return nil, ErrUnsupportedEndpoint
return nil, ErrUnsupportedServiceType
}
}
Loading
Loading