Annotation: Optionally allow storing longer annotation tags (#54754)

* Annotation: Optionally allow longer annotation tags

* Do not accept configuration lower than today's default (500)

* Apply suggestion from code review
This commit is contained in:
Sofia Papagiannaki 2022-09-23 13:04:41 +03:00 committed by GitHub
parent 883c7a802b
commit d0e7765c6a
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
10 changed files with 101 additions and 22 deletions

View File

@ -945,6 +945,10 @@ max_annotations_to_keep =
# Configures the batch size for the annotation clean-up job. This setting is used for dashboard, API, and alert annotations.
cleanupjob_batchsize = 100
# Enforces the maximum allowed length of the tags for any newly introduced annotations. It can be between 500 and 4096 inclusive (which is the respective's column length). Default value is 500.
# Setting it to a higher value would impact performance therefore is not recommended.
tags_length = 500
[annotations.dashboard]
# Dashboard annotations means that annotations are associated with the dashboard they are created on.

View File

@ -914,6 +914,10 @@
# Configures the batch size for the annotation clean-up job. This setting is used for dashboard, API, and alert annotations.
;cleanupjob_batchsize = 100
# Enforces the maximum allowed length of the tags for any newly introduced annotations. It can be between 500 and 4096 inclusive (which is the respective's column length). Default value is 500.
# Setting it to a higher value would impact performance therefore is not recommended.
;tags_length = 500
[annotations.dashboard]
# Dashboard annotations means that annotations are associated with the dashboard they are created on.

View File

@ -1387,6 +1387,10 @@ Configures max number of alert annotations that Grafana stores. Default value is
Configures the batch size for the annotation clean-up job. This setting is used for dashboard, API, and alert annotations.
### tags_length
Enforces the maximum allowed length of the tags for any newly introduced annotations. It can be between 500 and 4096 (inclusive). Default value is 500. Setting it to a higher value would impact performance therefore is not recommended.
## [annotations.dashboard]
Dashboard annotations means that annotations are associated with the dashboard they are created on.

View File

@ -154,7 +154,7 @@ func (hs *HTTPServer) PostAnnotation(c *models.ReqContext) response.Response {
if errors.Is(err, annotations.ErrTimerangeMissing) {
return response.Error(400, "Failed to save annotation", err)
}
return response.Error(500, "Failed to save annotation", err)
return response.ErrOrFallback(500, "Failed to save annotation", err)
}
startID := item.Id
@ -229,7 +229,7 @@ func (hs *HTTPServer) PostGraphiteAnnotation(c *models.ReqContext) response.Resp
}
if err := hs.annotationsRepo.Save(c.Req.Context(), &item); err != nil {
return response.Error(500, "Failed to save Graphite annotation", err)
return response.ErrOrFallback(500, "Failed to save Graphite annotation", err)
}
return response.JSON(http.StatusOK, util.DynMap{
@ -281,7 +281,7 @@ func (hs *HTTPServer) UpdateAnnotation(c *models.ReqContext) response.Response {
}
if err := hs.annotationsRepo.Update(c.Req.Context(), &item); err != nil {
return response.Error(500, "Failed to update annotation", err)
return response.ErrOrFallback(500, "Failed to update annotation", err)
}
return response.Success("Annotation updated")
@ -347,7 +347,7 @@ func (hs *HTTPServer) PatchAnnotation(c *models.ReqContext) response.Response {
}
if err := hs.annotationsRepo.Update(c.Req.Context(), &existing); err != nil {
return response.Error(500, "Failed to update annotation", err)
return response.ErrOrFallback(500, "Failed to update annotation", err)
}
return response.Success("Annotation patched")

View File

@ -5,10 +5,12 @@ import (
"errors"
"github.com/grafana/grafana/pkg/setting"
"github.com/grafana/grafana/pkg/util/errutil"
)
var (
ErrTimerangeMissing = errors.New("missing timerange")
ErrTimerangeMissing = errors.New("missing timerange")
ErrBaseTagLimitExceeded = errutil.NewBase(errutil.StatusBadRequest, "annotations.tag-limit-exceeded", errutil.WithPublicMessage("Tags length exceeds the maximum allowed."))
)
type Repository interface {

View File

@ -17,10 +17,11 @@ type RepositoryImpl struct {
func ProvideService(db db.DB, cfg *setting.Cfg, tagService tag.Service) *RepositoryImpl {
return &RepositoryImpl{
store: &xormRepositoryImpl{
cfg: cfg,
db: db,
log: log.New("annotations"),
tagService: tagService,
cfg: cfg,
db: db,
log: log.New("annotations"),
tagService: tagService,
maximumTagsLength: cfg.AnnotationMaximumTagsLength,
},
}
}

View File

@ -41,10 +41,11 @@ func validateTimeRange(item *annotations.Item) error {
}
type xormRepositoryImpl struct {
cfg *setting.Cfg
db db.DB
log log.Logger
tagService tag.Service
cfg *setting.Cfg
db db.DB
log log.Logger
maximumTagsLength int64
tagService tag.Service
}
func (r *xormRepositoryImpl) Add(ctx context.Context, item *annotations.Item) error {
@ -55,7 +56,7 @@ func (r *xormRepositoryImpl) Add(ctx context.Context, item *annotations.Item) er
if item.Epoch == 0 {
item.Epoch = item.Created
}
if err := validateTimeRange(item); err != nil {
if err := r.validateItem(item); err != nil {
return err
}
@ -106,10 +107,6 @@ func (r *xormRepositoryImpl) Update(ctx context.Context, item *annotations.Item)
existing.EpochEnd = item.EpochEnd
}
if err := validateTimeRange(existing); err != nil {
return err
}
if item.Tags != nil {
tags, err := r.tagService.EnsureTagsExist(ctx, tag.ParseTagPairs(item.Tags))
if err != nil {
@ -127,6 +124,10 @@ func (r *xormRepositoryImpl) Update(ctx context.Context, item *annotations.Item)
existing.Tags = item.Tags
if err := r.validateItem(existing); err != nil {
return err
}
_, err = sess.Table("annotation").ID(existing.Id).Cols("epoch", "text", "epoch_end", "updated", "tags").Update(existing)
return err
})
@ -379,6 +380,33 @@ func (r *xormRepositoryImpl) GetTags(ctx context.Context, query *annotations.Tag
return annotations.FindTagsResult{Tags: tags}, nil
}
func (r *xormRepositoryImpl) validateItem(item *annotations.Item) error {
if err := validateTimeRange(item); err != nil {
return err
}
if err := r.validateTagsLength(item); err != nil {
return err
}
return nil
}
func (r *xormRepositoryImpl) validateTagsLength(item *annotations.Item) error {
estimatedTagsLength := 1 // leading: [
for i, t := range item.Tags {
if i == 0 {
estimatedTagsLength += len(t) + 2 // quotes
} else {
estimatedTagsLength += len(t) + 3 // leading comma and quotes
}
}
estimatedTagsLength += 1 // trailing: ]
if estimatedTagsLength > int(r.maximumTagsLength) {
return annotations.ErrBaseTagLimitExceeded.Errorf("tags length (%d) exceeds the maximum allowed (%d): modify the configuration to increase it", estimatedTagsLength, r.maximumTagsLength)
}
return nil
}
func (r *xormRepositoryImpl) CleanAnnotations(ctx context.Context, cfg setting.AnnotationCleanupSettings, annotationType string) (int64, error) {
var totalAffected int64
if cfg.MaxAge > 0 {

View File

@ -3,6 +3,7 @@ package annotationsimpl
import (
"context"
"fmt"
"strings"
"testing"
"github.com/grafana/grafana/pkg/infra/log"
@ -28,7 +29,8 @@ func TestIntegrationAnnotations(t *testing.T) {
t.Skip("skipping integration test")
}
sql := sqlstore.InitTestDB(t)
repo := xormRepositoryImpl{db: sql, cfg: setting.NewCfg(), log: log.New("annotation.test"), tagService: tagimpl.ProvideService(sql)}
var maximumTagsLength int64 = 60
repo := xormRepositoryImpl{db: sql, cfg: setting.NewCfg(), log: log.New("annotation.test"), tagService: tagimpl.ProvideService(sql), maximumTagsLength: maximumTagsLength}
testUser := &user.SignedInUser{
OrgID: 1,
@ -148,6 +150,18 @@ func TestIntegrationAnnotations(t *testing.T) {
assert.Equal(t, items[0].Updated, items[0].Created)
})
badAnnotation := &annotations.Item{
OrgId: 1,
UserId: 1,
Text: "rollback",
Type: "",
Epoch: 17,
Tags: []string{strings.Repeat("a", int(maximumTagsLength+1))},
}
err = repo.Add(context.Background(), badAnnotation)
require.Error(t, err)
require.ErrorIs(t, err, annotations.ErrBaseTagLimitExceeded)
t.Run("Can query for annotation by id", func(t *testing.T) {
items, err := repo.Get(context.Background(), &annotations.ItemQuery{
OrgId: 1,
@ -394,7 +408,8 @@ func TestIntegrationAnnotationListingWithRBAC(t *testing.T) {
t.Skip("skipping integration test")
}
sql := sqlstore.InitTestDB(t, sqlstore.InitTestDBOpt{})
repo := xormRepositoryImpl{db: sql, cfg: setting.NewCfg(), log: log.New("annotation.test"), tagService: tagimpl.ProvideService(sql)}
var maximumTagsLength int64 = 60
repo := xormRepositoryImpl{db: sql, cfg: setting.NewCfg(), log: log.New("annotation.test"), tagService: tagimpl.ProvideService(sql), maximumTagsLength: maximumTagsLength}
dashboardStore := dashboardstore.ProvideDashboardStore(sql, featuremgmt.WithFeatures(), tagimpl.ProvideService(sql))
testDashboard1 := models.SaveDashboardCommand{

View File

@ -178,6 +178,10 @@ func addAnnotationMig(mg *Migrator) {
mg.AddMigration("Add index for alert_id on annotation table", NewAddIndexMigration(table, &Index{
Cols: []string{"alert_id"}, Type: IndexType,
}))
mg.AddMigration("Increase tags column to length 4096", NewRawSQLMigration("").
Postgres("ALTER TABLE annotation ALTER COLUMN tags TYPE VARCHAR(4096);").
Mysql("ALTER TABLE annotation MODIFY tags VARCHAR(4096);"))
}
type AddMakeRegionSingleRowMigration struct {

View File

@ -374,6 +374,7 @@ type Cfg struct {
// Annotations
AnnotationCleanupJobBatchSize int64
AnnotationMaximumTagsLength int64
AlertingAnnotationCleanupSetting AnnotationCleanupSettings
DashboardAnnotationCleanupSettings AnnotationCleanupSettings
APIAnnotationCleanupSettings AnnotationCleanupSettings
@ -601,9 +602,20 @@ func (cfg *Cfg) readGrafanaEnvironmentMetrics() error {
return nil
}
func (cfg *Cfg) readAnnotationSettings() {
func (cfg *Cfg) readAnnotationSettings() error {
section := cfg.Raw.Section("annotations")
cfg.AnnotationCleanupJobBatchSize = section.Key("cleanupjob_batchsize").MustInt64(100)
cfg.AnnotationMaximumTagsLength = section.Key("tags_length").MustInt64(500)
switch {
case cfg.AnnotationMaximumTagsLength > 4096:
// ensure that the configuration does not exceed the respective column size
return fmt.Errorf("[annotations.tags_length] configuration exceeds the maximum allowed (4096)")
case cfg.AnnotationMaximumTagsLength > 500:
cfg.Logger.Info("[annotations.tags_length] has been increased from its default value; this may affect the performance", "tagLength", cfg.AnnotationMaximumTagsLength)
case cfg.AnnotationMaximumTagsLength < 500:
cfg.Logger.Warn("[annotations.tags_length] is too low; the minimum allowed (500) is enforced")
cfg.AnnotationMaximumTagsLength = 500
}
dashboardAnnotation := cfg.Raw.Section("annotations.dashboard")
apiIAnnotation := cfg.Raw.Section("annotations.api")
@ -624,6 +636,8 @@ func (cfg *Cfg) readAnnotationSettings() {
cfg.AlertingAnnotationCleanupSetting = newAnnotationCleanupSettings(alertingSection, "max_annotation_age")
cfg.DashboardAnnotationCleanupSettings = newAnnotationCleanupSettings(dashboardAnnotation, "max_age")
cfg.APIAnnotationCleanupSettings = newAnnotationCleanupSettings(apiIAnnotation, "max_age")
return nil
}
func (cfg *Cfg) readExpressionsSettings() {
@ -1020,7 +1034,10 @@ func (cfg *Cfg) Load(args CommandLineArgs) error {
cfg.readSessionConfig()
cfg.readSmtpSettings()
cfg.readQuotaSettings()
cfg.readAnnotationSettings()
if err := cfg.readAnnotationSettings(); err != nil {
return err
}
cfg.readExpressionsSettings()
if err := cfg.readGrafanaEnvironmentMetrics(); err != nil {
return err