Skip to content

Commit

Permalink
Use UUID instead of ULID
Browse files Browse the repository at this point in the history
Implements spec change open-telemetry/opamp-spec#186

The example server still accepts old-style ULID agent instances to
demonstrate how this change can be handled in a backward compatible way.
  • Loading branch information
tigrannajaryan committed Apr 17, 2024
1 parent c7fc585 commit 5fe68c8
Show file tree
Hide file tree
Showing 23 changed files with 176 additions and 118 deletions.
36 changes: 20 additions & 16 deletions client/clientimpl_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,15 +5,14 @@ import (
"crypto/tls"
"crypto/x509"
"fmt"
"math/rand"
"net/http"
"net/http/httptest"
"net/url"
"sync/atomic"
"testing"
"time"

ulid "github.com/oklog/ulid/v2"
"github.com/google/uuid"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
"google.golang.org/protobuf/proto"
Expand Down Expand Up @@ -94,10 +93,17 @@ func eventually(t *testing.T, f func() bool) {
assert.Eventually(t, f, 5*time.Second, 10*time.Millisecond)
}

func newInstanceUid(t *testing.T) types.InstanceUid {
uid, err := uuid.NewV7()
require.NoError(t, err)
b, err := uid.MarshalBinary()
require.NoError(t, err)
return types.InstanceUid(b)
}

func prepareSettings(t *testing.T, settings *types.StartSettings, c OpAMPClient) {
// Autogenerate instance id.
entropy := ulid.Monotonic(rand.New(rand.NewSource(99)), 0)
settings.InstanceUid = ulid.MustNew(ulid.Timestamp(time.Now()), entropy).String()
settings.InstanceUid = newInstanceUid(t)

// Make sure correct URL scheme is used, based on the type of the OpAMP client.
u, err := url.Parse(settings.OpAMPServerURL)
Expand Down Expand Up @@ -150,7 +156,7 @@ func TestInvalidInstanceId(t *testing.T) {
testClients(t, func(t *testing.T, client OpAMPClient) {
settings := createNoServerSettings()
prepareClient(t, &settings, client)
settings.InstanceUid = "invalidid"
settings.InstanceUid = types.InstanceUid{}

err := client.Start(context.Background(), settings)
assert.Error(t, err)
Expand Down Expand Up @@ -624,9 +630,7 @@ func TestAgentIdentification(t *testing.T) {
testClients(t, func(t *testing.T, client OpAMPClient) {
// Start a server.
srv := internal.StartMockServer(t)
newInstanceUid := ulid.MustNew(
ulid.Timestamp(time.Now()), ulid.Monotonic(rand.New(rand.NewSource(0)), 0),
)
newInstanceUid := newInstanceUid(t)
var rcvAgentInstanceUid atomic.Value
var sentInvalidId atomic.Bool
srv.OnMessage = func(msg *protobufs.AgentToServer) *protobufs.ServerToAgent {
Expand All @@ -636,7 +640,7 @@ func TestAgentIdentification(t *testing.T) {
InstanceUid: msg.InstanceUid,
AgentIdentification: &protobufs.AgentIdentification{
// If we sent the invalid one first, send a valid one now
NewInstanceUid: newInstanceUid.String(),
NewInstanceUid: newInstanceUid[:],
},
}
}
Expand All @@ -645,7 +649,7 @@ func TestAgentIdentification(t *testing.T) {
InstanceUid: msg.InstanceUid,
AgentIdentification: &protobufs.AgentIdentification{
// Start by sending an invalid id forcing an error.
NewInstanceUid: "",
NewInstanceUid: nil,
},
}
}
Expand All @@ -662,11 +666,11 @@ func TestAgentIdentification(t *testing.T) {
eventually(
t,
func() bool {
instanceUid, ok := rcvAgentInstanceUid.Load().(string)
instanceUid, ok := rcvAgentInstanceUid.Load().([]byte)
if !ok {
return false
}
return instanceUid == oldInstanceUid
return types.InstanceUid(instanceUid) == oldInstanceUid
},
)

Expand All @@ -677,11 +681,11 @@ func TestAgentIdentification(t *testing.T) {
eventually(
t,
func() bool {
instanceUid, ok := rcvAgentInstanceUid.Load().(string)
instanceUid, ok := rcvAgentInstanceUid.Load().([]byte)
if !ok {
return false
}
return instanceUid == oldInstanceUid
return types.InstanceUid(instanceUid) == oldInstanceUid
},
)

Expand All @@ -693,11 +697,11 @@ func TestAgentIdentification(t *testing.T) {
eventually(
t,
func() bool {
instanceUid, ok := rcvAgentInstanceUid.Load().(string)
instanceUid, ok := rcvAgentInstanceUid.Load().([]byte)
if !ok {
return false
}
return instanceUid == newInstanceUid.String()
return types.InstanceUid(instanceUid) == newInstanceUid
},
)

Expand Down
8 changes: 4 additions & 4 deletions client/internal/receivedprocessor.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ package internal

import (
"context"
"errors"
"fmt"

"github.com/open-telemetry/opamp-go/client/types"
"github.com/open-telemetry/opamp-go/protobufs"
Expand Down Expand Up @@ -225,13 +225,13 @@ func (r *receivedProcessor) processErrorResponse(ctx context.Context, body *prot
}

func (r *receivedProcessor) rcvAgentIdentification(ctx context.Context, agentId *protobufs.AgentIdentification) error {
if agentId.NewInstanceUid == "" {
err := errors.New("empty instance uid is not allowed")
if len(agentId.NewInstanceUid) != 16 {
err := fmt.Errorf("instance uid must be 16 bytes but is %d bytes long", len(agentId.NewInstanceUid))
r.logger.Debugf(ctx, err.Error())
return err
}

err := r.sender.SetInstanceUid(agentId.NewInstanceUid)
err := r.sender.SetInstanceUid(types.InstanceUid(agentId.NewInstanceUid))
if err != nil {
r.logger.Errorf(ctx, "Error while setting instance uid: %v", err)
return err
Expand Down
15 changes: 6 additions & 9 deletions client/internal/sender.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ package internal
import (
"errors"

"github.com/oklog/ulid/v2"
"github.com/open-telemetry/opamp-go/client/types"
"github.com/open-telemetry/opamp-go/protobufs"
)

Expand All @@ -21,7 +21,7 @@ type Sender interface {
ScheduleSend()

// SetInstanceUid sets a new instanceUid to be used for all subsequent messages to be sent.
SetInstanceUid(instanceUid string) error
SetInstanceUid(instanceUid types.InstanceUid) error
}

// SenderCommon is partial Sender implementation that is common between WebSocket and plain
Expand Down Expand Up @@ -65,18 +65,15 @@ func (h *SenderCommon) NextMessage() *NextMessage {
// SetInstanceUid sets a new instanceUid to be used for all subsequent messages to be sent.
// Can be called concurrently, normally is called when a message is received from the
// Server that instructs us to change our instance UID.
func (h *SenderCommon) SetInstanceUid(instanceUid string) error {
if instanceUid == "" {
func (h *SenderCommon) SetInstanceUid(instanceUid types.InstanceUid) error {
var emptyUid types.InstanceUid
if instanceUid == emptyUid {
return errors.New("cannot set instance uid to empty value")
}

if _, err := ulid.ParseStrict(instanceUid); err != nil {
return err
}

h.nextMessage.Update(
func(msg *protobufs.AgentToServer) {
msg.InstanceUid = instanceUid
msg.InstanceUid = instanceUid[:]
})

return nil
Expand Down
2 changes: 1 addition & 1 deletion client/internal/wsreceiver_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -157,7 +157,7 @@ func TestDecodeMessage(t *testing.T) {
msgsToTest := []*protobufs.ServerToAgent{
{}, // Empty message
{
InstanceUid: "abcd",
InstanceUid: []byte("0123456789123456"),
},
}

Expand Down
3 changes: 3 additions & 0 deletions client/types/instanceid.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
package types

type InstanceUid [16]byte
2 changes: 1 addition & 1 deletion client/types/startsettings.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ type StartSettings struct {
TLSConfig *tls.Config

// Agent information.
InstanceUid string
InstanceUid InstanceUid

// Callbacks that the client will call after Start() returns nil.
Callbacks Callbacks
Expand Down
1 change: 1 addition & 0 deletions go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ require (
require (
github.com/davecgh/go-spew v1.1.1 // indirect
github.com/google/go-cmp v0.5.6 // indirect
github.com/google/uuid v1.6.0 // indirect
github.com/pmezard/go-difflib v1.0.0 // indirect
golang.org/x/net v0.17.0 // indirect
golang.org/x/xerrors v0.0.0-20200804184101-5ec99f83aff1 // indirect
Expand Down
2 changes: 2 additions & 0 deletions go.sum
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@ github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c
github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38=
github.com/google/go-cmp v0.5.6 h1:BKbKCqvP6I+rmFHt06ZmyQtvB8xAkWdhFyr0ZUNZcxQ=
github.com/google/go-cmp v0.5.6/go.mod h1:v8dTdLbMG2kIc/vJvl+f65V22dbkXbowE6jgT/gNBxE=
github.com/google/uuid v1.6.0 h1:NIvaJDMOsjHA8n1jAhLSgzrAzy1Hgr+hNrb57e+94F0=
github.com/google/uuid v1.6.0/go.mod h1:TIyPZe4MgqvfeYDBFedMoGGpEw/LqOeaOT+nhxU+yHo=
github.com/gorilla/websocket v1.5.1 h1:gmztn0JnHVt9JZquRuzLw3g4wouNVzKL15iLr/zn/QY=
github.com/gorilla/websocket v1.5.1/go.mod h1:x3kM2JMyaluk02fnUJpQuwD2dCS5NDG2ZHL0uE0tcaY=
github.com/oklog/ulid/v2 v2.1.0 h1:+9lhoxAP56we25tyYETBBY1YLA2SaoLvUFgrP2miPJU=
Expand Down
30 changes: 16 additions & 14 deletions internal/examples/agent/agent/agent.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,16 +10,14 @@ import (
"crypto/x509/pkix"
"encoding/pem"
"fmt"
"math/rand"
"os"
"runtime"
"sort"
"time"

"github.com/google/uuid"
"github.com/knadh/koanf"
"github.com/knadh/koanf/parsers/yaml"
"github.com/knadh/koanf/providers/rawbytes"
"github.com/oklog/ulid/v2"

"github.com/open-telemetry/opamp-go/client"
"github.com/open-telemetry/opamp-go/client/types"
Expand Down Expand Up @@ -54,7 +52,7 @@ type Agent struct {

effectiveConfig string

instanceId ulid.ULID
instanceId uuid.UUID

agentDescription *protobufs.AgentDescription

Expand Down Expand Up @@ -82,7 +80,7 @@ func NewAgent(logger types.Logger, agentType string, agentVersion string) *Agent

agent.createAgentIdentity()
agent.logger.Debugf(context.Background(), "Agent starting, id=%v, type=%s, version=%s.",
agent.instanceId.String(), agentType, agentVersion)
agent.instanceId, agentType, agentVersion)

agent.loadLocalConfig()
if err := agent.connect(); err != nil {
Expand All @@ -107,7 +105,7 @@ func (agent *Agent) connect() error {
settings := types.StartSettings{
OpAMPServerURL: "wss://127.0.0.1:4320/v1/opamp",
TLSConfig: tlsConfig,
InstanceUid: agent.instanceId.String(),
InstanceUid: types.InstanceUid(agent.instanceId),
Callbacks: types.CallbacksStruct{
OnConnectFunc: func(ctx context.Context) {
agent.logger.Debugf(ctx, "Connected to the server.")
Expand Down Expand Up @@ -167,8 +165,11 @@ func (agent *Agent) disconnect(ctx context.Context) {

func (agent *Agent) createAgentIdentity() {
// Generate instance id.
entropy := ulid.Monotonic(rand.New(rand.NewSource(0)), 0)
agent.instanceId = ulid.MustNew(ulid.Timestamp(time.Now()), entropy)
uid, err := uuid.NewV7()
if err != nil {
panic(err)
}
agent.instanceId = uid

hostname, _ := os.Hostname()

Expand Down Expand Up @@ -209,10 +210,10 @@ func (agent *Agent) createAgentIdentity() {
}
}

func (agent *Agent) updateAgentIdentity(ctx context.Context, instanceId ulid.ULID) {
func (agent *Agent) updateAgentIdentity(ctx context.Context, instanceId uuid.UUID) {
agent.logger.Debugf(ctx, "Agent identify is being changed from id=%v to id=%v",
agent.instanceId.String(),
instanceId.String())
agent.instanceId,
instanceId)
agent.instanceId = instanceId

if agent.metricReporter != nil {
Expand Down Expand Up @@ -459,11 +460,12 @@ func (agent *Agent) onMessage(ctx context.Context, msg *types.MessageData) {
}

if msg.AgentIdentification != nil {
newInstanceId, err := ulid.Parse(msg.AgentIdentification.NewInstanceUid)
uid, err := uuid.FromBytes(msg.AgentIdentification.NewInstanceUid)
if err != nil {
agent.logger.Errorf(ctx, err.Error())
agent.logger.Errorf(ctx, "invalid NewInstanceUid: %v", err)
return
}
agent.updateAgentIdentity(ctx, newInstanceId)
agent.updateAgentIdentity(ctx, uid)
}

if configChanged {
Expand Down
4 changes: 2 additions & 2 deletions internal/examples/agent/agent/metricreporter.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import (
"os"
"time"

"github.com/oklog/ulid/v2"
"github.com/google/uuid"
"github.com/shirou/gopsutil/process"
"go.opentelemetry.io/otel"
"go.opentelemetry.io/otel/attribute"
Expand Down Expand Up @@ -46,7 +46,7 @@ func NewMetricReporter(
dest *protobufs.TelemetryConnectionSettings,
agentType string,
agentVersion string,
instanceId ulid.ULID,
instanceId uuid.UUID,
) (*MetricReporter, error) {

// Check the destination credentials to make sure they look like a valid OTLP/HTTP
Expand Down
1 change: 1 addition & 0 deletions internal/examples/go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ require (
github.com/go-logr/stdr v1.2.2 // indirect
github.com/go-ole/go-ole v1.2.6 // indirect
github.com/golang/protobuf v1.5.3 // indirect
github.com/google/uuid v1.6.0 // indirect
github.com/gorilla/websocket v1.5.1 // indirect
github.com/grpc-ecosystem/grpc-gateway/v2 v2.19.1 // indirect
github.com/mitchellh/copystructure v1.2.0 // indirect
Expand Down
2 changes: 2 additions & 0 deletions internal/examples/go.sum
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,8 @@ github.com/google/go-cmp v0.5.4/go.mod h1:v8dTdLbMG2kIc/vJvl+f65V22dbkXbowE6jgT/
github.com/google/go-cmp v0.5.5/go.mod h1:v8dTdLbMG2kIc/vJvl+f65V22dbkXbowE6jgT/gNBxE=
github.com/google/go-cmp v0.5.6/go.mod h1:v8dTdLbMG2kIc/vJvl+f65V22dbkXbowE6jgT/gNBxE=
github.com/google/go-cmp v0.6.0 h1:ofyhxvXcZhMsU5ulbFiLKl/XBFqE1GSq7atu8tAmTRI=
github.com/google/uuid v1.6.0 h1:NIvaJDMOsjHA8n1jAhLSgzrAzy1Hgr+hNrb57e+94F0=
github.com/google/uuid v1.6.0/go.mod h1:TIyPZe4MgqvfeYDBFedMoGGpEw/LqOeaOT+nhxU+yHo=
github.com/gorilla/websocket v1.5.1 h1:gmztn0JnHVt9JZquRuzLw3g4wouNVzKL15iLr/zn/QY=
github.com/gorilla/websocket v1.5.1/go.mod h1:x3kM2JMyaluk02fnUJpQuwD2dCS5NDG2ZHL0uE0tcaY=
github.com/grpc-ecosystem/grpc-gateway/v2 v2.19.1 h1:/c3QmbOGMGTOumP2iT/rCwB7b0QDGLKzqOmktBjT+Is=
Expand Down
7 changes: 5 additions & 2 deletions internal/examples/server/data/agent.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import (
"sync"
"time"

"github.com/google/uuid"
"google.golang.org/protobuf/proto"

"github.com/open-telemetry/opamp-go/internal/examples/server/certman"
Expand All @@ -23,7 +24,8 @@ type Agent struct {
// Some fields in this struct are exported so that we can render them in the UI.

// Agent's instance id. This is an immutable field.
InstanceId InstanceId
InstanceId InstanceId
InstanceIdStr string

// Connection to the Agent.
conn types.Connection
Expand Down Expand Up @@ -60,7 +62,7 @@ func NewAgent(
instanceId InstanceId,
conn types.Connection,
) *Agent {
agent := &Agent{InstanceId: instanceId, conn: conn}
agent := &Agent{InstanceId: instanceId, InstanceIdStr: uuid.UUID(instanceId).String(), conn: conn}
tslConn, ok := conn.Connection().(*tls.Conn)
if ok {
// Client is using TLS connection.
Expand All @@ -84,6 +86,7 @@ func (agent *Agent) CloneReadonly() *Agent {
defer agent.mux.RUnlock()
return &Agent{
InstanceId: agent.InstanceId,
InstanceIdStr: uuid.UUID(agent.InstanceId).String(),
Status: proto.Clone(agent.Status).(*protobufs.AgentToServer),
EffectiveConfig: agent.EffectiveConfig,
CustomInstanceConfig: agent.CustomInstanceConfig,
Expand Down
4 changes: 3 additions & 1 deletion internal/examples/server/data/instanceid.go
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
package data

type InstanceId string
import "github.com/google/uuid"

type InstanceId uuid.UUID
Loading

0 comments on commit 5fe68c8

Please sign in to comment.