diff --git a/pkg/admin/client.go b/pkg/admin/client.go index f731de6cc3..76b9727573 100644 --- a/pkg/admin/client.go +++ b/pkg/admin/client.go @@ -5,11 +5,13 @@ import ( "encoding/json" "errors" "fmt" - "github.com/pyroscope-io/pyroscope/pkg/server" "io" "net/http" "time" + "github.com/pyroscope-io/pyroscope/pkg/api" + "github.com/pyroscope-io/pyroscope/pkg/model/appmetadata" + "github.com/hashicorp/go-multierror" ) @@ -57,20 +59,23 @@ func (c *Client) GetAppsNames() (names AppNames, err error) { return names, multierror.Append(ErrStatusCodeNotOK, err) } - err = json.NewDecoder(resp.Body).Decode(&names) + // Strip all data except fqname + var apps []appmetadata.ApplicationMetadata + err = json.NewDecoder(resp.Body).Decode(&apps) if err != nil { return names, multierror.Append(ErrDecodingResponse, err) } - return names, nil + appNames := make([]string, len(apps)) + for i, appName := range apps { + appNames[i] = appName.FQName + } + + return appNames, nil } func (c *Client) DeleteApp(name string) (err error) { - // we are kinda robbing here - // since the server and client are defined in the same package - payload := server.DeleteAppInput{ - Name: name, - } + payload := api.DeleteAppInput{Name: name} marshalledPayload, err := json.Marshal(payload) if err != nil { diff --git a/pkg/admin/client_test.go b/pkg/admin/client_test.go index 88e24e0319..de0fd3f56d 100644 --- a/pkg/admin/client_test.go +++ b/pkg/admin/client_test.go @@ -60,7 +60,7 @@ var _ = Describe("client", func() { handler = http.NewServeMux() handler.HandleFunc("/v1/apps", func(w http.ResponseWriter, r *http.Request) { w.WriteHeader(200) - _, _ = fmt.Fprintf(w, `["name1", "name2"]`) + _, _ = fmt.Fprintf(w, `[{ "name": "name1"}, {"name": "name2" }]`) }) }) diff --git a/pkg/admin/controller.go b/pkg/admin/controller.go index 361585c60e..8dcf8b8911 100644 --- a/pkg/admin/controller.go +++ b/pkg/admin/controller.go @@ -6,26 +6,21 @@ import ( "context" "encoding/json" "fmt" - "github.com/pyroscope-io/pyroscope/pkg/server/httputils" - pstorage "github.com/pyroscope-io/pyroscope/pkg/storage" "net/http" + "github.com/pyroscope-io/pyroscope/pkg/model/appmetadata" + "github.com/pyroscope-io/pyroscope/pkg/server/httputils" + "github.com/gorilla/mux" "github.com/sirupsen/logrus" "github.com/pyroscope-io/pyroscope/pkg/model" ) -type Storage interface { - pstorage.AppNameGetter - pstorage.AppGetter - pstorage.AppDeleter -} - type Controller struct { log *logrus.Logger httpUtils httputils.Utils - storage Storage + appService ApplicationListerAndDeleter userService UserService storageService StorageService } @@ -38,15 +33,20 @@ type StorageService interface { Cleanup(ctx context.Context) error } +type ApplicationListerAndDeleter interface { + List(ctx context.Context) (apps []appmetadata.ApplicationMetadata, err error) + Delete(ctx context.Context, name string) error +} + func NewController( log *logrus.Logger, - storage Storage, + appService ApplicationListerAndDeleter, userService UserService, storageService StorageService) *Controller { return &Controller{ log: log, - storage: storage, + appService: appService, userService: userService, storageService: storageService, } diff --git a/pkg/admin/controller_test.go b/pkg/admin/controller_test.go index 83d3932cad..076fc3185a 100644 --- a/pkg/admin/controller_test.go +++ b/pkg/admin/controller_test.go @@ -1,3 +1,4 @@ +// TODO: move most of these tests to pkg/api package admin_test import ( @@ -5,12 +6,13 @@ import ( "context" "encoding/json" "fmt" - "github.com/pyroscope-io/pyroscope/pkg/server" - "github.com/pyroscope-io/pyroscope/pkg/storage" "io" "net/http" "net/http/httptest" + "github.com/pyroscope-io/pyroscope/pkg/api" + "github.com/pyroscope-io/pyroscope/pkg/model/appmetadata" + . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" "github.com/sirupsen/logrus/hooks/test" @@ -20,20 +22,15 @@ import ( ) type mockStorage struct { - getAppNamesResult []string - getAppsResult storage.GetAppsOutput - deleteResult error -} - -func (m mockStorage) GetAppNames(ctx context.Context) []string { - return m.getAppNamesResult + getAppsResult []appmetadata.ApplicationMetadata + deleteResult error } -func (m mockStorage) GetApps(ctx context.Context) (storage.GetAppsOutput, error) { +func (m mockStorage) List(ctx context.Context) ([]appmetadata.ApplicationMetadata, error) { return m.getAppsResult, nil } -func (m mockStorage) DeleteApp(ctx context.Context, appname string) error { +func (m mockStorage) Delete(ctx context.Context, appname string) error { return m.deleteResult } @@ -53,13 +50,16 @@ var _ = Describe("controller", func() { Describe("/v1/apps", func() { var svr *admin.Server var response *httptest.ResponseRecorder - var storage admin.Storage + var appSvc admin.ApplicationListerAndDeleter // create a server BeforeEach(func() { - storage = mockStorage{ - getAppNamesResult: []string{"app1", "app2"}, - deleteResult: nil, + appSvc = mockStorage{ + getAppsResult: []appmetadata.ApplicationMetadata{ + {FQName: "app1"}, + {FQName: "app2"}, + }, + deleteResult: nil, } }) @@ -67,7 +67,7 @@ var _ = Describe("controller", func() { // create a null logger, since we aren't interested logger, _ := test.NewNullLogger() - ctrl := admin.NewController(logger, storage, mockUserService{}, mockStorageService{}) + ctrl := admin.NewController(logger, appSvc, mockUserService{}, mockStorageService{}) httpServer := &admin.UdsHTTPServer{} server, err := admin.NewServer(logger, ctrl, httpServer) @@ -88,7 +88,7 @@ var _ = Describe("controller", func() { Expect(err).To(BeNil()) Expect(response.Code).To(Equal(http.StatusOK)) - Expect(string(body)).To(Equal(`["app1","app2"] + Expect(string(body)).To(Equal(`[{"name":"app1"},{"name":"app2"}] `)) }) @@ -100,7 +100,7 @@ var _ = Describe("controller", func() { It("returns StatusOK", func() { // we are kinda robbing here // since the server and client are defined in the same package - payload := server.DeleteAppInput{Name: "myapp"} + payload := api.DeleteAppInput{Name: "myapp"} marshalledPayload, err := json.Marshal(payload) request, err := http.NewRequest(http.MethodDelete, "/v1/apps", bytes.NewBuffer(marshalledPayload)) Expect(err).ToNot(HaveOccurred()) @@ -112,6 +112,12 @@ var _ = Describe("controller", func() { Context("when there's an error", func() { Context("with the payload", func() { + BeforeEach(func() { + appSvc = mockStorage{ + deleteResult: model.ValidationError{}, + } + }) + It("returns BadRequest", func() { request, err := http.NewRequest(http.MethodDelete, "/v1/apps", bytes.NewBuffer([]byte(``))) Expect(err).ToNot(HaveOccurred()) @@ -123,7 +129,7 @@ var _ = Describe("controller", func() { Context("with the server", func() { BeforeEach(func() { - storage = mockStorage{ + appSvc = mockStorage{ deleteResult: fmt.Errorf("error"), } }) @@ -131,7 +137,7 @@ var _ = Describe("controller", func() { It("returns InternalServerError", func() { // we are kinda robbing here // since the server and client are defined in the same package - payload := server.DeleteAppInput{Name: "myapp"} + payload := api.DeleteAppInput{Name: "myapp"} marshalledPayload, err := json.Marshal(payload) request, err := http.NewRequest(http.MethodDelete, "/v1/apps", bytes.NewBuffer(marshalledPayload)) Expect(err).ToNot(HaveOccurred()) diff --git a/pkg/admin/server.go b/pkg/admin/server.go index c74a64d91f..a61b023145 100644 --- a/pkg/admin/server.go +++ b/pkg/admin/server.go @@ -1,11 +1,12 @@ package admin import ( - "github.com/pyroscope-io/pyroscope/pkg/server" - "github.com/pyroscope-io/pyroscope/pkg/server/httputils" "net/http" "os" + "github.com/pyroscope-io/pyroscope/pkg/api" + "github.com/pyroscope-io/pyroscope/pkg/server/httputils" + "github.com/gorilla/handlers" "github.com/gorilla/mux" "github.com/sirupsen/logrus" @@ -42,8 +43,10 @@ func NewServer(logger *logrus.Logger, ctrl *Controller, httpServer HTTPServer) ( httpUtils := httputils.NewDefaultHelper(logger) // Routes - r.HandleFunc("/v1/apps", server.NewGetAppNamesHandler(ctrl.storage, httpUtils)).Methods("GET") - r.HandleFunc("/v1/apps", server.NewDeleteAppHandler(ctrl.storage, httpUtils)).Methods("DELETE") + applicationsHandler := api.NewApplicationsHandler(ctrl.appService, httpUtils) + r.HandleFunc("/v1/apps", applicationsHandler.GetApps).Methods("GET") + r.HandleFunc("/v1/apps", applicationsHandler.DeleteApp).Methods("DELETE") + r.HandleFunc("/v1/users/{username}", ctrl.UpdateUserHandler).Methods("PATCH") r.HandleFunc("/v1/storage/cleanup", ctrl.StorageCleanupHandler).Methods("PUT") diff --git a/pkg/api/application.go b/pkg/api/application.go deleted file mode 100644 index acb16ea1f6..0000000000 --- a/pkg/api/application.go +++ /dev/null @@ -1,14 +0,0 @@ -package api - -import ( - "context" - - "github.com/pyroscope-io/pyroscope/pkg/storage" -) - -type ApplicationService interface { - List(context.Context) ([]storage.ApplicationMetadata, error) // GET /apps - Get(ctx context.Context, name string) (storage.ApplicationMetadata, error) // GET /apps/{name} - CreateOrUpdate(context.Context, storage.ApplicationMetadata) error // PUT /apps // Should be idempotent. - Delete(ctx context.Context, name string) error // DELETE /apps/{name} -} diff --git a/pkg/api/applications.go b/pkg/api/applications.go new file mode 100644 index 0000000000..f6a0d774ed --- /dev/null +++ b/pkg/api/applications.go @@ -0,0 +1,60 @@ +package api + +import ( + "context" + "encoding/json" + "fmt" + "net/http" + + "github.com/pyroscope-io/pyroscope/pkg/model/appmetadata" + "github.com/pyroscope-io/pyroscope/pkg/server/httputils" +) + +type ApplicationListerAndDeleter interface { + List(context.Context) ([]appmetadata.ApplicationMetadata, error) + Delete(ctx context.Context, name string) error +} + +type ApplicationsHandler struct { + svc ApplicationListerAndDeleter + httpUtils httputils.Utils +} + +func NewApplicationsHandler(svc ApplicationListerAndDeleter, httpUtils httputils.Utils) *ApplicationsHandler { + return &ApplicationsHandler{ + svc: svc, + httpUtils: httpUtils, + } +} + +func (h *ApplicationsHandler) GetApps(w http.ResponseWriter, r *http.Request) { + apps, err := h.svc.List(r.Context()) + if err != nil { + h.httpUtils.HandleError(r, w, err) + return + } + + w.WriteHeader(http.StatusOK) + h.httpUtils.WriteResponseJSON(r, w, apps) +} + +type DeleteAppInput struct { + Name string `json:"name"` +} + +func (h *ApplicationsHandler) DeleteApp(w http.ResponseWriter, r *http.Request) { + var payload DeleteAppInput + + fmt.Println("calling delete app") + err := json.NewDecoder(r.Body).Decode(&payload) + if err != nil { + h.httpUtils.HandleError(r, w, httputils.JSONError{Err: err}) + return + } + + err = h.svc.Delete(r.Context(), payload.Name) + if err != nil { + h.httpUtils.HandleError(r, w, err) + return + } +} diff --git a/pkg/api/router/router.go b/pkg/api/router/router.go index 1f81540d6e..ca00a95709 100644 --- a/pkg/api/router/router.go +++ b/pkg/api/router/router.go @@ -24,6 +24,7 @@ type Services struct { api.APIKeyService api.AnnotationsService api.AdhocService + api.ApplicationListerAndDeleter } func New(m *mux.Router, s Services) *Router { @@ -94,3 +95,12 @@ func (r *Router) RegisterAdhocHandlers() { x.Methods(http.MethodGet).PathPrefix("/diff/{left:[0-9a-f]+}/{right:[0-9a-f]+}").HandlerFunc(h.GetProfileDiff) x.Methods(http.MethodPost).PathPrefix("/upload").HandlerFunc(h.Upload) } + +func (r *Router) RegisterApplicationHandlers() { + h := api.NewApplicationsHandler(r.ApplicationListerAndDeleter, httputils.NewDefaultHelper(r.Logger)) + authorizer := authz.NewAuthorizer(r.Services.Logger, httputils.NewDefaultHelper(r.Logger)) + + x := r.PathPrefix("/apps").Subrouter() + x.Methods(http.MethodGet).HandlerFunc(h.GetApps) + x.Methods(http.MethodDelete).Handler(authorizer.RequireAdminRole(http.HandlerFunc(h.DeleteApp))) +} diff --git a/pkg/cli/app_metadata_migration.go b/pkg/cli/app_metadata_migration.go index bdda0e4222..854fe58c94 100644 --- a/pkg/cli/app_metadata_migration.go +++ b/pkg/cli/app_metadata_migration.go @@ -4,7 +4,7 @@ import ( "context" "github.com/hashicorp/go-multierror" - "github.com/pyroscope-io/pyroscope/pkg/storage" + "github.com/pyroscope-io/pyroscope/pkg/model/appmetadata" "github.com/sirupsen/logrus" ) @@ -13,8 +13,8 @@ type AppNamesGetter interface { } type AppMetadataSaver interface { - CreateOrUpdate(_ context.Context, _ storage.ApplicationMetadata) error - List(context.Context) ([]storage.ApplicationMetadata, error) + CreateOrUpdate(_ context.Context, _ appmetadata.ApplicationMetadata) error + List(context.Context) ([]appmetadata.ApplicationMetadata, error) } type AppMetadataMigrator struct { @@ -45,7 +45,7 @@ func (m *AppMetadataMigrator) Migrate() (err error) { // TODO skip if not necessary // Convert slice -> map - appMap := make(map[string]storage.ApplicationMetadata) + appMap := make(map[string]appmetadata.ApplicationMetadata) for _, a := range apps { appMap[a.FQName] = a } @@ -55,7 +55,7 @@ func (m *AppMetadataMigrator) Migrate() (err error) { if _, ok := appMap[a]; !ok { logrus.Info("Migrating app: ", a) // Write to MetadataSaver - saveErr := m.appMetadataSaver.CreateOrUpdate(ctx, storage.ApplicationMetadata{ + saveErr := m.appMetadataSaver.CreateOrUpdate(ctx, appmetadata.ApplicationMetadata{ FQName: a, }) if err != nil { diff --git a/pkg/cli/server.go b/pkg/cli/server.go index a5c90059c9..2a836e286a 100644 --- a/pkg/cli/server.go +++ b/pkg/cli/server.go @@ -114,17 +114,19 @@ func newServerService(c *config.Server) (*serverService, error) { } } - migrator := NewAppMetadataMigrator(logger, svc.storage, service.NewApplicationMetadataService(svc.database.DB())) + appMetadataSvc := service.NewApplicationMetadataService(svc.database.DB()) + migrator := NewAppMetadataMigrator(logger, svc.storage, appMetadataSvc) err = migrator.Migrate() if err != nil { svc.logger.Error(err) } + appSvc := service.NewApplicationService(appMetadataSvc, svc.storage) // this needs to happen after storage is initiated! if svc.config.EnableExperimentalAdmin { socketPath := svc.config.AdminSocketPath userService := service.NewUserService(svc.database.DB()) - adminController := admin.NewController(svc.logger, svc.storage, userService, svc.storage) + adminController := admin.NewController(svc.logger, appSvc, userService, svc.storage) httpClient, err := admin.NewHTTPOverUDSClient(socketPath) if err != nil { return nil, fmt.Errorf("admin: %w", err) diff --git a/pkg/model/appmetadata/application_metadata.go b/pkg/model/appmetadata/application_metadata.go new file mode 100644 index 0000000000..fd73ac0b04 --- /dev/null +++ b/pkg/model/appmetadata/application_metadata.go @@ -0,0 +1,13 @@ +package appmetadata + +import "github.com/pyroscope-io/pyroscope/pkg/storage/metadata" + +type ApplicationMetadata struct { + // Fully Qualified Name. Eg app.cpu ({__name__}.{profile_type}) + FQName string `gorm:"index,unique;not null;default:null" json:"name"` + + SpyName string `json:"spyName,omitempty"` + SampleRate uint32 `json:"sampleRate,omitempty"` + Units metadata.Units `json:"units,omitempty"` + AggregationType metadata.AggregationType `json:"-"` +} diff --git a/pkg/server/apps.go b/pkg/server/apps.go deleted file mode 100644 index 13d4bf7fe6..0000000000 --- a/pkg/server/apps.go +++ /dev/null @@ -1,92 +0,0 @@ -package server - -import ( - "context" - "encoding/json" - "net/http" - - "github.com/pyroscope-io/pyroscope/pkg/server/httputils" - "github.com/pyroscope-io/pyroscope/pkg/service" - "github.com/pyroscope-io/pyroscope/pkg/storage" -) - -type AppInfo struct { - Name string `json:"name"` -} - -type DeleteAppInput struct { - Name string `json:"name"` -} - -func (ctrl *Controller) getAppsHandler() http.HandlerFunc { - svc := service.NewApplicationMetadataService(ctrl.db) - return NewGetAppsHandler(svc, ctrl.httpUtils) -} - -type AppGetter interface { - List(ctx context.Context) (apps []storage.ApplicationMetadata, err error) -} - -func NewGetAppsHandler(s AppGetter, httpUtils httputils.Utils) func(w http.ResponseWriter, r *http.Request) { - return func(w http.ResponseWriter, r *http.Request) { - apps, err := s.List(r.Context()) - if err != nil { - httpUtils.HandleError(r, w, err) - return - } - - // apps, err := s.GetApps(r.Context()) - // if err != nil { - // httpUtils.WriteError(r, w, http.StatusInternalServerError, err, "") - // return - // } - // res := make([]AppInfo, 0, len(apps.Apps)) - // for _, app := range apps.Apps { - // it := AppInfo{ - // Name: app.Name, - // } - // res = append(res, it) - // } - w.WriteHeader(http.StatusOK) - httpUtils.WriteResponseJSON(r, w, apps) - } -} - -func (ctrl *Controller) getAppNames() http.HandlerFunc { - return NewGetAppNamesHandler(ctrl.storage, ctrl.httpUtils) -} - -func NewGetAppNamesHandler(s storage.AppNameGetter, httpUtils httputils.Utils) func(w http.ResponseWriter, r *http.Request) { - return func(w http.ResponseWriter, r *http.Request) { - appNames := s.GetAppNames(r.Context()) - w.WriteHeader(http.StatusOK) - httpUtils.WriteResponseJSON(r, w, appNames) - } -} - -func (ctrl *Controller) deleteAppsHandler() http.HandlerFunc { - return NewDeleteAppHandler(ctrl.storage, ctrl.httpUtils) -} - -func NewDeleteAppHandler(s storage.AppDeleter, httpUtils httputils.Utils) func(w http.ResponseWriter, r *http.Request) { - return func(w http.ResponseWriter, r *http.Request) { - var payload DeleteAppInput - - err := json.NewDecoder(r.Body).Decode(&payload) - if err != nil { - httpUtils.WriteError(r, w, http.StatusBadRequest, err, "") - return - } - - err = s.DeleteApp(r.Context(), payload.Name) - if err != nil { - // TODO how to distinguish - // it was a bad request - // or an internal server error - httpUtils.WriteError(r, w, http.StatusInternalServerError, err, "") - return - } - - w.WriteHeader(http.StatusOK) - } -} diff --git a/pkg/server/controller.go b/pkg/server/controller.go index 748b27057a..797dcef2bf 100644 --- a/pkg/server/controller.go +++ b/pkg/server/controller.go @@ -195,6 +195,9 @@ func (ctrl *Controller) serverMux() (http.Handler, error) { ctrl.userService = service.NewUserService(ctrl.db) ctrl.annotationsService = service.NewAnnotationsService(ctrl.db) + appMetadataSvc := service.NewApplicationMetadataService(ctrl.db) + appSvc := service.NewApplicationService(appMetadataSvc, ctrl.storage) + apiRouter := router.New(r.PathPrefix("/api").Subrouter(), router.Services{ Logger: ctrl.log, APIKeyService: service.NewAPIKeyService(ctrl.db), @@ -204,6 +207,7 @@ func (ctrl *Controller) serverMux() (http.Handler, error) { AdhocService: service.NewAdhocService( ctrl.config.MaxNodesRender, ctrl.config.AdhocDataPath), + ApplicationListerAndDeleter: appSvc, }) apiRouter.Use( @@ -220,7 +224,6 @@ func (ctrl *Controller) serverMux() (http.Handler, error) { } // FIXME: not optimal, unify this with the remoteReadHandler at the top - authorizer := authz.NewAuthorizer(ctrl.log, httputils.NewDefaultHelper(ctrl.log)) appsRouter := apiRouter.PathPrefix("/apps").Subrouter() if ctrl.config.RemoteRead.Enabled { h, err := ctrl.remoteReadHandler(ctrl.config.RemoteRead) @@ -231,8 +234,7 @@ func (ctrl *Controller) serverMux() (http.Handler, error) { appsRouter.Methods(http.MethodDelete).Handler(h) } } else { - appsRouter.Methods(http.MethodGet).Handler(ctrl.getAppsHandler()) - appsRouter.Methods(http.MethodDelete).Handler(authorizer.RequireAdminRole(ctrl.deleteAppsHandler())) + apiRouter.RegisterApplicationHandlers() } ingestRouter := r.Path("/ingest").Subrouter() diff --git a/pkg/service/application.go b/pkg/service/application.go new file mode 100644 index 0000000000..a0ec32b6f9 --- /dev/null +++ b/pkg/service/application.go @@ -0,0 +1,42 @@ +package service + +import ( + "context" + + "github.com/pyroscope-io/pyroscope/pkg/model" +) + +type AppDeleter interface { + DeleteApp(ctx context.Context, appName string) error +} + +type ApplicationService struct { + ApplicationMetadataService + storageDeleter AppDeleter +} + +// NewApplicationService creates an ApplicationService +// Which just delegates to its underlying ApplicationMetadataService +// Except when deleting, which is then forward to both ApplicationMetadataService and storageDeleter +func NewApplicationService(appMetadataSvc ApplicationMetadataService, storageDeleter AppDeleter) ApplicationService { + return ApplicationService{ + appMetadataSvc, + storageDeleter, + } +} + +// Delete deletes apps from both storage and ApplicationMetadata +// It first deletes from storage and only then deletes its metadata +func (svc ApplicationService) Delete(ctx context.Context, name string) error { + err := model.ValidateAppName(name) + if err != nil { + return err + } + + err = svc.storageDeleter.DeleteApp(ctx, name) + if err != nil { + return err + } + + return svc.ApplicationMetadataService.Delete(ctx, name) +} diff --git a/pkg/service/application_metadata.go b/pkg/service/application_metadata.go index 70cf9cb408..813981f2cd 100644 --- a/pkg/service/application_metadata.go +++ b/pkg/service/application_metadata.go @@ -5,7 +5,7 @@ import ( "errors" "github.com/pyroscope-io/pyroscope/pkg/model" - "github.com/pyroscope-io/pyroscope/pkg/storage" + "github.com/pyroscope-io/pyroscope/pkg/model/appmetadata" "gorm.io/gorm" ) @@ -17,14 +17,14 @@ func NewApplicationMetadataService(db *gorm.DB) ApplicationMetadataService { return ApplicationMetadataService{db: db} } -func (svc ApplicationMetadataService) List(ctx context.Context) (apps []storage.ApplicationMetadata, err error) { +func (svc ApplicationMetadataService) List(ctx context.Context) (apps []appmetadata.ApplicationMetadata, err error) { tx := svc.db.WithContext(ctx) result := tx.Find(&apps) return apps, result.Error } -func (svc ApplicationMetadataService) Get(ctx context.Context, name string) (storage.ApplicationMetadata, error) { - app := storage.ApplicationMetadata{} +func (svc ApplicationMetadataService) Get(ctx context.Context, name string) (appmetadata.ApplicationMetadata, error) { + app := appmetadata.ApplicationMetadata{} if err := model.ValidateAppName(name); err != nil { return app, err } @@ -40,7 +40,7 @@ func (svc ApplicationMetadataService) Get(ctx context.Context, name string) (sto } } -func (svc ApplicationMetadataService) CreateOrUpdate(ctx context.Context, application storage.ApplicationMetadata) error { +func (svc ApplicationMetadataService) CreateOrUpdate(ctx context.Context, application appmetadata.ApplicationMetadata) error { if err := model.ValidateAppName(application.FQName); err != nil { return err } @@ -48,9 +48,9 @@ func (svc ApplicationMetadataService) CreateOrUpdate(ctx context.Context, applic tx := svc.db.WithContext(ctx) // Only update the field if it's populated - return tx.Where(storage.ApplicationMetadata{ + return tx.Where(appmetadata.ApplicationMetadata{ FQName: application.FQName, - }).Assign(application).FirstOrCreate(&storage.ApplicationMetadata{}).Error + }).Assign(application).FirstOrCreate(&appmetadata.ApplicationMetadata{}).Error } func (svc ApplicationMetadataService) Delete(ctx context.Context, name string) error { @@ -59,5 +59,5 @@ func (svc ApplicationMetadataService) Delete(ctx context.Context, name string) e } tx := svc.db.WithContext(ctx) - return tx.Where("fq_name = ?", name).Delete(storage.ApplicationMetadata{}).Error + return tx.Where("fq_name = ?", name).Delete(appmetadata.ApplicationMetadata{}).Error } diff --git a/pkg/service/application_metadata_cache.go b/pkg/service/application_metadata_cache.go index 064e8dfca8..d8e4aee4e4 100644 --- a/pkg/service/application_metadata_cache.go +++ b/pkg/service/application_metadata_cache.go @@ -5,11 +5,11 @@ import ( "reflect" "time" - "github.com/pyroscope-io/pyroscope/pkg/storage" + "github.com/pyroscope-io/pyroscope/pkg/model/appmetadata" ) type ApplicationMetadataWriter interface { - CreateOrUpdate(ctx context.Context, application storage.ApplicationMetadata) error + CreateOrUpdate(ctx context.Context, application appmetadata.ApplicationMetadata) error } type ApplicationMetadataCacheService struct { @@ -39,9 +39,9 @@ func NewApplicationMetadataCacheService(config ApplicationMetadataCacheServiceCo // * item is not in the cache // * data is different from what's in the cache // Otherwise it does nothing -func (svc *ApplicationMetadataCacheService) CreateOrUpdate(ctx context.Context, application storage.ApplicationMetadata) error { +func (svc *ApplicationMetadataCacheService) CreateOrUpdate(ctx context.Context, application appmetadata.ApplicationMetadata) error { if cachedApp, ok := svc.cache.get(application.FQName); ok { - if !svc.isTheSame(application, cachedApp.(storage.ApplicationMetadata)) { + if !svc.isTheSame(application, cachedApp.(appmetadata.ApplicationMetadata)) { return svc.writeToBoth(ctx, application) } return nil @@ -54,7 +54,7 @@ func (svc *ApplicationMetadataCacheService) CreateOrUpdate(ctx context.Context, } // writeToBoth writes to both the cache and the underlying service -func (svc *ApplicationMetadataCacheService) writeToBoth(ctx context.Context, application storage.ApplicationMetadata) error { +func (svc *ApplicationMetadataCacheService) writeToBoth(ctx context.Context, application appmetadata.ApplicationMetadata) error { if err := svc.appSvc.CreateOrUpdate(ctx, application); err != nil { return err } @@ -65,6 +65,6 @@ func (svc *ApplicationMetadataCacheService) writeToBoth(ctx context.Context, app // isTheSame check if 2 applications have the same data // TODO(eh-am): update to a more robust comparison function // See https://pkg.go.dev/reflect#DeepEqual for its drawbacks -func (*ApplicationMetadataCacheService) isTheSame(app1 storage.ApplicationMetadata, app2 storage.ApplicationMetadata) bool { +func (*ApplicationMetadataCacheService) isTheSame(app1 appmetadata.ApplicationMetadata, app2 appmetadata.ApplicationMetadata) bool { return reflect.DeepEqual(app1, app2) } diff --git a/pkg/service/application_metadata_cache_test.go b/pkg/service/application_metadata_cache_test.go index 4431c4a068..f20cc67662 100644 --- a/pkg/service/application_metadata_cache_test.go +++ b/pkg/service/application_metadata_cache_test.go @@ -5,16 +5,16 @@ import ( . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" + "github.com/pyroscope-io/pyroscope/pkg/model/appmetadata" "github.com/pyroscope-io/pyroscope/pkg/service" - "github.com/pyroscope-io/pyroscope/pkg/storage" "github.com/pyroscope-io/pyroscope/pkg/storage/metadata" ) type mockApplicationWriter struct { - onWrite func(application storage.ApplicationMetadata) error + onWrite func(application appmetadata.ApplicationMetadata) error } -func (svc *mockApplicationWriter) CreateOrUpdate(ctx context.Context, application storage.ApplicationMetadata) error { +func (svc *mockApplicationWriter) CreateOrUpdate(ctx context.Context, application appmetadata.ApplicationMetadata) error { return svc.onWrite(application) } @@ -23,7 +23,7 @@ var _ = Describe("ApplicationWriteCacheService", func() { BeforeEach(s.BeforeEach) AfterEach(s.AfterEach) - sampleApp := storage.ApplicationMetadata{ + sampleApp := appmetadata.ApplicationMetadata{ FQName: "myapp", SampleRate: 100, SpyName: "gospy", @@ -42,7 +42,7 @@ var _ = Describe("ApplicationWriteCacheService", func() { When("cache is empty", func() { BeforeEach(func() { m = mockApplicationWriter{ - onWrite: func(application storage.ApplicationMetadata) error { + onWrite: func(application appmetadata.ApplicationMetadata) error { Expect(application).To(Equal(sampleApp)) return nil }, @@ -61,7 +61,7 @@ var _ = Describe("ApplicationWriteCacheService", func() { BeforeEach(func() { n = 0 m = mockApplicationWriter{ - onWrite: func(application storage.ApplicationMetadata) error { + onWrite: func(application appmetadata.ApplicationMetadata) error { n = n + 1 return nil }, diff --git a/pkg/service/application_metadata_test.go b/pkg/service/application_metadata_test.go index f85dcb9d04..e088813fdc 100644 --- a/pkg/service/application_metadata_test.go +++ b/pkg/service/application_metadata_test.go @@ -6,8 +6,8 @@ import ( . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" "github.com/pyroscope-io/pyroscope/pkg/model" + "github.com/pyroscope-io/pyroscope/pkg/model/appmetadata" "github.com/pyroscope-io/pyroscope/pkg/service" - "github.com/pyroscope-io/pyroscope/pkg/storage" "github.com/pyroscope-io/pyroscope/pkg/storage/metadata" ) @@ -21,7 +21,7 @@ var _ = Describe("ApplicationMetadataService", func() { svc = service.NewApplicationMetadataService(s.DB()) }) - app := storage.ApplicationMetadata{ + app := appmetadata.ApplicationMetadata{ FQName: "myapp", SampleRate: 100, SpyName: "gospy", @@ -29,7 +29,7 @@ var _ = Describe("ApplicationMetadataService", func() { AggregationType: metadata.AverageAggregationType, } - assertNumOfApps := func(num int) []storage.ApplicationMetadata { + assertNumOfApps := func(num int) []appmetadata.ApplicationMetadata { apps, err := svc.List(context.TODO()) Expect(err).ToNot(HaveOccurred()) Expect(len(apps)).To(Equal(num)) @@ -40,7 +40,7 @@ var _ = Describe("ApplicationMetadataService", func() { ctx := context.TODO() // Create - err := svc.CreateOrUpdate(ctx, storage.ApplicationMetadata{FQName: ""}) + err := svc.CreateOrUpdate(ctx, appmetadata.ApplicationMetadata{FQName: ""}) Expect(err).To(HaveOccurred()) Expect(model.IsValidationError(err)).To(BeTrue()) @@ -73,7 +73,7 @@ var _ = Describe("ApplicationMetadataService", func() { err := svc.CreateOrUpdate(ctx, app) Expect(err).ToNot(HaveOccurred()) - err = svc.CreateOrUpdate(ctx, storage.ApplicationMetadata{ + err = svc.CreateOrUpdate(ctx, appmetadata.ApplicationMetadata{ FQName: app.FQName, SampleRate: 101, }) diff --git a/pkg/storage/application_service_noop.go b/pkg/storage/application_service_noop.go index 7b30117ede..db2a8a688c 100644 --- a/pkg/storage/application_service_noop.go +++ b/pkg/storage/application_service_noop.go @@ -1,20 +1,24 @@ package storage -import "context" +import ( + "context" + + "github.com/pyroscope-io/pyroscope/pkg/model/appmetadata" +) // NoopApplicationMetadataService implements same methods as ApplicationMetadataService // But it doesn't do anything when called type NoopApplicationMetadataService struct{} -func (NoopApplicationMetadataService) CreateOrUpdate(context.Context, ApplicationMetadata) error { +func (NoopApplicationMetadataService) CreateOrUpdate(context.Context, appmetadata.ApplicationMetadata) error { return nil } -func (NoopApplicationMetadataService) List(context.Context, ApplicationMetadata) (apps []ApplicationMetadata, err error) { +func (NoopApplicationMetadataService) List(context.Context, appmetadata.ApplicationMetadata) (apps []appmetadata.ApplicationMetadata, err error) { return apps, err } -func (NoopApplicationMetadataService) Get(context.Context, string) (app ApplicationMetadata, err error) { +func (NoopApplicationMetadataService) Get(context.Context, string) (app appmetadata.ApplicationMetadata, err error) { return app, err } diff --git a/pkg/storage/storage.go b/pkg/storage/storage.go index 8bca3153ce..d367017194 100644 --- a/pkg/storage/storage.go +++ b/pkg/storage/storage.go @@ -14,6 +14,7 @@ import ( "github.com/sirupsen/logrus" "github.com/pyroscope-io/pyroscope/pkg/health" + "github.com/pyroscope-io/pyroscope/pkg/model/appmetadata" "github.com/pyroscope-io/pyroscope/pkg/storage/cache" "github.com/pyroscope-io/pyroscope/pkg/storage/labels" "github.com/pyroscope-io/pyroscope/pkg/storage/segment" @@ -84,7 +85,7 @@ type SampleObserver interface { // ApplicationMetadataSaver saves application metadata type ApplicationMetadataSaver interface { - CreateOrUpdate(ctx context.Context, application ApplicationMetadata) error + CreateOrUpdate(ctx context.Context, application appmetadata.ApplicationMetadata) error } func New(c *Config, logger *logrus.Logger, reg prometheus.Registerer, hc *health.Controller, appSvc ApplicationMetadataSaver) (*Storage, error) { diff --git a/pkg/storage/storage_labels.go b/pkg/storage/storage_labels.go index 33c35d83c7..55337f1794 100644 --- a/pkg/storage/storage_labels.go +++ b/pkg/storage/storage_labels.go @@ -99,14 +99,3 @@ func (s *Storage) GetAppNames(ctx context.Context) []string { return appNames } - -func (s *Storage) GetApps(ctx context.Context) (GetAppsOutput, error) { - r := GetAppsOutput{} - for _, appName := range s.GetAppNames(ctx) { - a := AppInfo{ - Name: appName, - } - r.Apps = append(r.Apps, a) - } - return r, nil -} diff --git a/pkg/storage/storage_put.go b/pkg/storage/storage_put.go index 2fc7d60bb6..65a60ba2cc 100644 --- a/pkg/storage/storage_put.go +++ b/pkg/storage/storage_put.go @@ -8,6 +8,7 @@ import ( "github.com/sirupsen/logrus" + "github.com/pyroscope-io/pyroscope/pkg/model/appmetadata" "github.com/pyroscope-io/pyroscope/pkg/storage/dimension" "github.com/pyroscope-io/pyroscope/pkg/storage/metadata" "github.com/pyroscope-io/pyroscope/pkg/storage/segment" @@ -33,7 +34,7 @@ func (s *Storage) Put(ctx context.Context, pi *PutInput) error { return errRetention } - if err := s.appSvc.CreateOrUpdate(ctx, ApplicationMetadata{ + if err := s.appSvc.CreateOrUpdate(ctx, appmetadata.ApplicationMetadata{ FQName: pi.Key.AppName(), SpyName: pi.SpyName, SampleRate: pi.SampleRate, diff --git a/pkg/storage/types.go b/pkg/storage/types.go index 5037763399..5b0df52602 100644 --- a/pkg/storage/types.go +++ b/pkg/storage/types.go @@ -8,7 +8,6 @@ import ( "github.com/dgraph-io/badger/v2" "github.com/pyroscope-io/pyroscope/pkg/storage/cache" - "github.com/pyroscope-io/pyroscope/pkg/storage/metadata" "github.com/pyroscope-io/pyroscope/pkg/util/bytesize" ) @@ -58,35 +57,11 @@ type GetLabelValuesByQueryOutput struct { Values []string } -type AppInfo struct { - Name string -} - -type GetAppsOutput struct { - Apps []AppInfo -} - -type DeleteAppInput struct { - Name string -} - type LabelValuesGetter interface { GetValues(ctx context.Context, key string, cb func(v string) bool) GetValuesByQuery(ctx context.Context, in GetLabelValuesByQueryInput) (GetLabelValuesByQueryOutput, error) } -type AppNameGetter interface { - GetAppNames(ctx context.Context) []string -} - -type AppGetter interface { - GetApps(ctx context.Context) (GetAppsOutput, error) -} - -type AppDeleter interface { - DeleteApp(ctx context.Context, appName string) error -} - // Other functions from storage.Storage: // type Backend interface { // Put(ctx context.Context, pi *PutInput) error @@ -132,13 +107,3 @@ type BadgerDBWithCache interface { CacheInstance() *cache.Cache Name() string } - -type ApplicationMetadata struct { - // Fully Qualified Name. Eg app.cpu ({__name__}.{profile_type}) - FQName string `gorm:"index,unique;not null;default:null" json:"name"` - - SpyName string `json:"spyName,omitempty"` - SampleRate uint32 `json:"sampleRate,omitempty"` - Units metadata.Units `json:"units,omitempty"` - AggregationType metadata.AggregationType `json:"-"` -}