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

Small changes around node IDs and trace IDs #2202

Merged
merged 2 commits into from
Jan 10, 2025
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
5 changes: 2 additions & 3 deletions internal/graph/check.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ import (
"fmt"
"time"

"github.com/google/uuid"
"github.com/prometheus/client_golang/prometheus"
"github.com/samber/lo"
"go.opentelemetry.io/otel"
Expand Down Expand Up @@ -128,7 +127,7 @@ func (cc *ConcurrentChecker) Check(ctx context.Context, req ValidatedCheckReques
if debugInfo == nil {
debugInfo = &v1.DebugInformation{
Check: &v1.CheckDebugTrace{
TraceId: uuid.NewString(),
TraceId: NewTraceID(),
SourceId: nodeID,
},
}
Expand Down Expand Up @@ -1328,7 +1327,7 @@ func combineResponseMetadata(ctx context.Context, existing *v1.ResponseMeta, res

debugInfo := &v1.DebugInformation{
Check: &v1.CheckDebugTrace{
TraceId: uuid.NewString(),
TraceId: NewTraceID(),
SourceId: nodeID,
},
}
Expand Down
13 changes: 13 additions & 0 deletions internal/graph/traceid.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
package graph

import (
"github.com/google/uuid"
)

// NewTraceID generates a new trace ID. The trace IDs will only be unique with
// a single dispatch request tree and should not be used for any other purpose.
// This function currently uses the UUID library to generate a new trace ID,
// which means it should not be invoked from performance-critical code paths.
func NewTraceID() string {
return uuid.NewString()
}
3 changes: 1 addition & 2 deletions internal/services/v1/bulkcheck.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ import (
"time"

v1 "github.com/authzed/authzed-go/proto/authzed/api/v1"
"github.com/google/uuid"
"github.com/jzelinskie/stringz"
"google.golang.org/grpc/status"
"google.golang.org/protobuf/types/known/durationpb"
Expand Down Expand Up @@ -199,7 +198,7 @@ func (bc *bulkChecker) checkBulkPermissions(ctx context.Context, req *v1.CheckBu
},
Results: localResults,
Duration: durationpb.New(time.Duration(0)),
TraceId: uuid.New().String(),
TraceId: graph.NewTraceID(),
SourceId: debugInfo.Check.SourceId,
},
}
Expand Down
11 changes: 10 additions & 1 deletion pkg/middleware/nodeid/nodeid.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,10 @@ package nodeid

import (
"context"
"fmt"
"os"

"github.com/cespare/xxhash/v2"
middleware "github.com/grpc-ecosystem/go-grpc-middleware/v2"
"google.golang.org/grpc"
)
Expand Down Expand Up @@ -40,7 +42,14 @@ func FromContext(ctx context.Context) (string, error) {
if err != nil {
return "", err
}
defaultNodeID = spiceDBPrefix + hostname

// Hash the hostname to get the final default node ID.
hasher := xxhash.New()
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Move this out of the function?

Copy link
Member Author

Choose a reason for hiding this comment

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

Since we cache the name globally after computation, I think this is fine

if _, err := hasher.WriteString(hostname); err != nil {
return "", fmt.Errorf("failed to hash hostname: %w", err)
}

defaultNodeID = spiceDBPrefix + fmt.Sprintf("%x", hasher.Sum(nil))
}

if err := setInContext(ctx, defaultNodeID); err != nil {
Expand Down
Loading