Skip to content

Commit

Permalink
refactor(probeservices): use better naming (#1628)
Browse files Browse the repository at this point in the history
Calling the endpoint base URL (e.g., `https://www.example.com/`)
"endpoint" could cause confusion because usually an API endpoint is
something like `https://www.example.com/api/v1/check-in`. To obviate
this semantic issue, this diff attempts to avoid calling base URLs as
endpoint throughout the tree.

I am going to account this work as part of
ooni/backend#754 because this is one of the
possibly issues on which we can account this work. The original
conversation with @ainghazal, which prodded me to make these changes,
was related to his feedback after implementing
#1625.
  • Loading branch information
bassosimone committed Jun 25, 2024
1 parent f10bad6 commit 65df439
Show file tree
Hide file tree
Showing 6 changed files with 86 additions and 83 deletions.
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) {
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")

// 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

0 comments on commit 65df439

Please sign in to comment.