Ad Hoc Profiles #2963
Ad Hoc Profiles #2963
Conversation
| uid := ulid.MustNew(ulid.Timestamp(adHocProfile.UploadedAt), rand.Reader) | ||
| id := strings.Join([]string{uid.String(), adHocProfile.Name}, "-") |
There was a problem hiding this comment.
Keys in the object storage will have the form 01HMXV8BF4EH71NBYZNPPVGJ2X-cpu.pprof, using ULID for the first segment to simplify embedding a timestamp. This is then used in the /List endpoint to return meaningful data without querying each individual profile.
pkg/sidekick/sidekick.go
Outdated
| type Sidekick struct { | ||
| services.Service | ||
|
|
||
| AdHocProfiles *adhocprofiles.AdHocProfiles |
There was a problem hiding this comment.
I could make AdHocProfiles a subservice perhaps, if we decide that having a Sidekick module still makes sense. This could come in in a second PR where we "move" tenant settings here.
pkg/adhocprofiles/adhocprofiles.go
Outdated
| return nil, connect.NewError(connect.CodeInvalidArgument, err) | ||
| } | ||
|
|
||
| bucket, err := a.getBucket(tenantID) |
There was a problem hiding this comment.
Shouldn't you use here ?
bucket, err := a.getBucketFromContext(ctx)
if err != nil {
return nil, err
}
There was a problem hiding this comment.
Using getBucket(tenant) saves a bit of work here, because we already extracted the tenantId a few lines above. Nothing major, but avoiding doing things twice.
| id, err := ulid.Parse(s[0:separatorIndex]) | ||
| if err != nil { | ||
| level.Warn(a.logger).Log("msg", "cannot parse ad hoc profile", "key", s, "err", err) | ||
| return nil |
There was a problem hiding this comment.
nit: may be we can continue here ? Otherwise a tenant can get corrupted and will never be able to list the rest ?
There was a problem hiding this comment.
we are inside bucket.Iter() here, returning nil continues the iteration
pkg/adhocprofiles/adhocprofiles.go
Outdated
|
|
||
| profiles := make([]*v1.AdHocProfilesProfileMetadata, 0) | ||
| err = bucket.Iter(ctx, "", func(s string) error { | ||
| separatorIndex := bytes.IndexByte([]byte(s), '-') |
There was a problem hiding this comment.
| separatorIndex := bytes.IndexByte([]byte(s), '-') | |
| separatorIndex := strings.IndexRune(s, '-') |
This is to avoid []byte conversion which copies the string, it's irrelevant here but I just wanted to let you know if you ever do that in a hot path be aware of string <-> []byte conversion.
There was a problem hiding this comment.
The suggested line is also shorter and more readable, thanks for that tip!
pkg/adhocprofiles/adhocprofiles.go
Outdated
| return a.getBucket(tenantID) | ||
| } | ||
|
|
||
| func (a *AdHocProfiles) getBucket(tenantID string) (objstore.Bucket, error) { |
There was a problem hiding this comment.
I don't think you need to cache buckets you could create one child per request and throw it away.
but may be there's a reason ?
There was a problem hiding this comment.
No reason, I was not sure about the overhead of creating buckets. Since this is not in a hot path, I can definitely simplify it.
Adds APIs for uploading and retrieving "ad hoc" profiles. Profiles are stored in the tenant object storage, in a new directory (
adhoc/) next tophlaredb/.The code for parsing the data and doing conversions is copied (with minor adaptations) from OG Pyroscope to maintain feature parity with the original Ad Hoc implementation.
Edit: As of ad79c09 the module is just
ad-hoc-profiles. Deployment considerations will be handled elsewhere.The only validation done in this PR is to make sure the profile is readable. In the future, we could also add per-tenant limits (e.g., storage usage).
TODO: