Skip to content

chore: make pair (timestamp, appName) unique #1513

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.

Sign up for GitHub

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

Merged
eh-am merged 6 commits into from
Sep 16, 2022
Merged

chore: make pair (timestamp, appName) unique #1513

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
6c8cde3
write migration
eh-am Sep 15, 2022
88bb4b1
refactor: annotation upsert
eh-am Sep 15, 2022
5d4eb0b
refactor migration
eh-am Sep 15, 2022
d0df55b
add comment
eh-am Sep 15, 2022
d9cdee2
fix upsert
eh-am Sep 15, 2022
88fe79e
update model
eh-am Sep 15, 2022
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
4 changes: 2 additions & 2 deletions pkg/model/annotation.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,8 @@ var (
)

type Annotation struct {
AppName string `gorm:"not null;default:null"`
Timestamp time.Time `form:"not null;default:null"`
AppName string `gorm:"index:idx_appname_timestamp,unique;not null;default:null"`
Timestamp time.Time `gorm:"index:idx_appname_timestamp,unique;not null;default:null"`
Content string `gorm:"not null;default:null"`
CreatedAt time.Time
UpdatedAt time.Time
Expand Down
17 changes: 12 additions & 5 deletions pkg/service/annotation.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import (

"github.com/pyroscope-io/pyroscope/pkg/model"
"gorm.io/gorm"
"gorm.io/gorm/clause"
)

type AnnotationsService struct{ db *gorm.DB }
Expand All @@ -30,11 +31,17 @@ func (svc AnnotationsService) CreateAnnotation(ctx context.Context, params model
tx := svc.db.WithContext(ctx)

// Upsert
if tx.Where(model.CreateAnnotation{
AppName: params.AppName,
Timestamp: params.Timestamp,
}).Updates(&u).RowsAffected == 0 {
return &u, tx.Create(&u).Error
if err := tx.Clauses(clause.OnConflict{
Columns: []clause.Column{
{Name: "app_name"},
{Name: "timestamp"},
},
// Update fields we care about
DoUpdates: clause.AssignmentColumns([]string{"app_name", "timestamp", "content"}),
// Update updateAt fields
UpdateAll: true,
}).Create(&u).Error; err != nil {
return nil, err
}
Comment on lines +34 to 45
Copy link
Collaborator Author

@eh-am eh-am Sep 15, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure this is an improvement.

There are a few things happening here:

  1. We need to tell which fields we are interested in conflicting (app_name, timestamp), otherwise it defaults to the primary key. I decided to not use a natural primary key here, since we may want to for eg modify an existing annotation (change appName for example)
  2. DoUpdates since we need to tell which columns we want to update. Otherwise it doesn't do anything.
  3. UpdateAll to tell it to update updateAt field. Here's where it gets confusing: I thought UpdateAll would update everything (duh), but it seems to ignore the other fields.

Copy link
Collaborator

@kolesnikovae kolesnikovae Sep 16, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's interesting – I was sure that UpdateAll does update all columns but primary keys.

Maybe we should update content and updated_at explicitly – just for clarity:

{
    Columns: ...,
    DoUpdates: clause.AssignmentColumns([]string{"content", "updated_at"}),
}

Copy link
Collaborator Author

@eh-am eh-am Sep 16, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did not work, I think it's due to how updated_at is generated dynamically by gorm.


return &u, nil
Expand Down
2 changes: 1 addition & 1 deletion pkg/service/annotation_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ var _ = Describe("AnnotationsService", func() {
Expect(annotation).ToNot(BeNil())
Expect(annotation.AppName).To(Equal("myapp"))
Expect(annotation.Content).To(Equal("mycontent"))
Expect(annotation.Timestamp).To(Equal(now))
Expect(annotation.Timestamp.Unix()).To(Equal(now.Unix()))

Expect(annotation.CreatedAt).ToNot(BeZero())
Expect(annotation.UpdatedAt).ToNot(BeZero())
Expand Down
18 changes: 18 additions & 0 deletions pkg/sqlstore/migrations/migrations.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ func Migrate(db *gorm.DB, c *config.Server) error {
createUserTableMigration(c.Auth.Internal.AdminUser),
createAPIKeyTableMigration(),
createAnnotationsTableMigration(),
addIndexesUniqueTableMigration(),
}).Migrate()
}

Expand Down Expand Up @@ -132,3 +133,20 @@ func createAnnotationsTableMigration() *gormigrate.Migration {
},
}
}

func addIndexesUniqueTableMigration() *gormigrate.Migration {
type annotation struct {
AppName string `gorm:"index:idx_appname_timestamp,unique;not null;default:null"`
Timestamp time.Time `gorm:"index:idx_appname_timestamp,unique;not null;default:null"`
Copy link
Collaborator

@kolesnikovae kolesnikovae Sep 16, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Previous declaration has a typo: form -> gorm

Timestamp time.Time `form:"not null;default:null"`

Copy link
Collaborator Author

@eh-am eh-am Sep 16, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed (and fixed it), but not in migration thought right?

}

return &gormigrate.Migration{
ID: "1663269650",
Migrate: func(tx *gorm.DB) error {
return tx.AutoMigrate(&annotation{})
},
Rollback: func(tx *gorm.DB) error {
return tx.Migrator().DropIndex(&annotation{}, "idx_appname_timestamp")
},
}
}