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

feat(annotations): allow creating multiple annotations for different apps #1562

Merged
merged 3 commits into from
Sep 28, 2022
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
107 changes: 86 additions & 21 deletions pkg/api/annotation.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,13 +3,15 @@ package api
import (
"context"
"encoding/json"
"errors"
"net/http"
"strconv"
"time"

"github.com/pyroscope-io/pyroscope/pkg/model"
"github.com/pyroscope-io/pyroscope/pkg/server/httputils"
"github.com/pyroscope-io/pyroscope/pkg/util/attime"
"golang.org/x/sync/errgroup"
)

type AnnotationsService interface {
Expand All @@ -29,15 +31,15 @@ func NewAnnotationsHandler(svc AnnotationsService, httpUtils httputils.Utils) *A
}

type CreateParams struct {
AppName string `json:"appName"`
Timestamp int64 `json:"timestamp"`
Content string `json:"content"`
AppName string `json:"appName"`
AppNames []string `json:"appNames"`
Timestamp int64 `json:"timestamp"`
Content string `json:"content"`
}

func (h *AnnotationsHandler) CreateAnnotation(w http.ResponseWriter, r *http.Request) {
var params CreateParams
if err := json.NewDecoder(r.Body).Decode(&params); err != nil {
h.httpUtils.WriteInternalServerError(r, w, err, "failed to unmarshal JSON")
params := h.validateAppNames(w, r)
if params == nil {
return
}

Expand All @@ -46,28 +48,91 @@ func (h *AnnotationsHandler) CreateAnnotation(w http.ResponseWriter, r *http.Req
timestamp = attime.Parse(strconv.FormatInt(params.Timestamp, 10))
}

annotation, err := h.svc.CreateAnnotation(r.Context(), model.CreateAnnotation{
AppName: params.AppName,
Timestamp: timestamp,
Content: params.Content,
})
if err != nil {
h.httpUtils.HandleError(r, w, err)
return
}

// TODO(eh-am): unify this with render.go
type annotationsResponse struct {
AppName string `json:"appName"`
Content string `json:"content"`
Timestamp int64 `json:"timestamp"`
}
annotationsResp := annotationsResponse{
AppName: annotation.AppName,
Content: annotation.Content,
Timestamp: annotation.Timestamp.Unix(),

createAnnotations := func(ctx context.Context, params *CreateParams) ([]annotationsResponse, error) {
g, ctx := errgroup.WithContext(ctx)

results := make([]annotationsResponse, len(params.AppNames))
for i, appName := range params.AppNames {
appName := appName
i := i
g.Go(func() error {
annotation, err := h.svc.CreateAnnotation(ctx, model.CreateAnnotation{
AppName: appName,
Timestamp: timestamp,
Content: params.Content,
})
if err != nil {
return err
}

results[i] = annotationsResponse{
AppName: annotation.AppName,
Content: annotation.Content,
Timestamp: annotation.Timestamp.Unix(),
}

return err
})
}

if err := g.Wait(); err != nil {
return nil, err
}
return results, nil
}

res, err := createAnnotations(r.Context(), params)
if err != nil {
h.httpUtils.HandleError(r, w, err)
return
}

w.WriteHeader(http.StatusCreated)
h.httpUtils.WriteResponseJSON(r, w, annotationsResp)
if len(res) == 1 {
h.httpUtils.WriteResponseJSON(r, w, res[0])
return
}

h.httpUtils.WriteResponseJSON(r, w, res)
}

// validateAppNames handles the different combinations between (`appName` and `appNames`)
// in the failure case, it returns nil and serves an error
// in the success case, it returns a `CreateParams` struct where `appNames` is ALWAYS populated with at least one appName
func (h *AnnotationsHandler) validateAppNames(w http.ResponseWriter, r *http.Request) *CreateParams {
var params CreateParams

if err := json.NewDecoder(r.Body).Decode(&params); err != nil {
h.httpUtils.WriteInternalServerError(r, w, err, "failed to unmarshal JSON")
return nil
}

// handling `appName` and `appNames`
// 1. Both are set, is invalid
if params.AppName != "" && len(params.AppNames) > 0 {
h.httpUtils.HandleError(r, w, model.ValidationError{errors.New("only one of 'appName' and 'appNames' can be specified")})
return nil
}

// 2. None are set
if params.AppName == "" && len(params.AppNames) <= 0 {
h.httpUtils.HandleError(r, w, model.ValidationError{errors.New("at least one of 'appName' and 'appNames' needs to be specified")})
return nil
}

// 3. Only appName is set
if params.AppName != "" && len(params.AppNames) <= 0 {
params.AppNames = append(params.AppNames, params.AppName)
return &params
}

// 4. Only appNames is set
return &params
}
74 changes: 74 additions & 0 deletions pkg/api/annotation_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,34 @@ var _ = Describe("AnnotationHandler", func() {
})
})

When("multiple appNames are passed", func() {
BeforeEach(func() {
svc = &mockService{
createAnnotationResponse: func(params model.CreateAnnotation) (*model.Annotation, error) {
return &model.Annotation{
AppName: params.AppName,
Content: "mycontent",
Timestamp: time.Unix(1662729099, 0),
}, nil
},
}

server = httptest.NewServer(newTestRouter(defaultUserCtx, router.Services{
Logger: logrus.StandardLogger(),
AnnotationsService: svc,
}))
})

It("creates correctly", func() {
url := server.URL + "/annotations"

expectResponse(newRequest(http.MethodPost, url,
"annotation/create_multiple_request.json"),
"annotation/create_multiple_response.json",
http.StatusCreated)
})
})

When("fields are invalid", func() {
BeforeEach(func() {
svc = &mockService{
Expand All @@ -116,5 +144,51 @@ var _ = Describe("AnnotationHandler", func() {
http.StatusBadRequest)
})
})

When("both 'appName' and 'appNames' are passed", func() {
BeforeEach(func() {
svc = &mockService{
createAnnotationResponse: func(params model.CreateAnnotation) (*model.Annotation, error) {
return nil, nil
},
}

server = httptest.NewServer(newTestRouter(defaultUserCtx, router.Services{
Logger: logrus.StandardLogger(),
AnnotationsService: svc,
}))
})
It("returns an error", func() {
url := server.URL + "/annotations"

expectResponse(newRequest(http.MethodPost, url,
"annotation/create_request_appName_appNames_error.json"),
"annotation/create_response_appName_appNames_error.json",
http.StatusBadRequest)
})
})

When("none of 'appName' and 'appNames' are passed", func() {
BeforeEach(func() {
svc = &mockService{
createAnnotationResponse: func(params model.CreateAnnotation) (*model.Annotation, error) {
return nil, nil
},
}

server = httptest.NewServer(newTestRouter(defaultUserCtx, router.Services{
Logger: logrus.StandardLogger(),
AnnotationsService: svc,
}))
})
It("returns an error", func() {
url := server.URL + "/annotations"

expectResponse(newRequest(http.MethodPost, url,
"annotation/create_request_appName_appNames_empty_error.json"),
"annotation/create_response_appName_appNames_empty_error.json",
http.StatusBadRequest)
})
})
})
})
5 changes: 5 additions & 0 deletions pkg/api/testdata/annotation/create_multiple_request.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
{
"appNames": ["myApp1.cpu", "myApp1.goroutines", "myApp1.alloc_objects"],
"content": "mycontent",
"timestamp": 1662729099
}
17 changes: 17 additions & 0 deletions pkg/api/testdata/annotation/create_multiple_response.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
[
{
"appName": "myApp1.cpu",
"content": "mycontent",
"timestamp": 1662729099
},
{
"appName": "myApp1.goroutines",
"content": "mycontent",
"timestamp": 1662729099
},
{
"appName": "myApp1.alloc_objects",
"content": "mycontent",
"timestamp": 1662729099
}
]
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
{
"content": "mycontent",
"timestamp": 1662729099
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
{
"appName": "myapp",
"appNames": ["myApp1.goroutines"],
"content": "mycontent",
"timestamp": 1662729099
}
4 changes: 3 additions & 1 deletion pkg/api/testdata/annotation/create_request_error.json
Original file line number Diff line number Diff line change
@@ -1 +1,3 @@
{}
{
"appName": "_"
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
{
"errors": ["at least one of 'appName' and 'appNames' needs to be specified"]
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
{
"errors": ["only one of 'appName' and 'appNames' can be specified"]
}