-
Notifications
You must be signed in to change notification settings - Fork 291
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
Add a metric tracking the selected replica #2231
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, just one suggestion with memdb.
if err != nil { | ||
return "", errors.New("could not parse datastore URL") | ||
} | ||
return u.Host + u.Path, nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note that returning host+path will lead to explosion of cardinality when having many spiced clusters in the same physical database. I do see the value in disambiguating telemetry across spicedb clusters, just something to note.
@@ -100,6 +100,10 @@ type snapshot struct { | |||
db *memdb.MemDB | |||
} | |||
|
|||
func (mdb *memdbDatastore) MetricsID() (string, error) { | |||
return "memdb-" + mdb.uniqueID, nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this almost certainly cause explosion in cadinality for these metrics. E.g. when MemDB SpiceDB is embedded in another process, each restart will bring different uniqueID. I'd personally just have it as memdb
- the scenario of multiple SpiceDB processes in memory is less likely, and the explosion in cardinality can have severe cost implications
Allows for understanding which replica(s) are being used