Skip to content

Add product-drop-timeout parameter #165

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

Draft
mpejcoch wants to merge 3 commits into
base: main
Choose a base branch
from
Draft

Add product-drop-timeout parameter #165

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6de0558
Add product-drop-timeout parameter
Aug 12, 2021
f7894c3
Merge branch 'master' into issue163_purge_old_products
mpejcoch Aug 13, 2021
a19a90f
Merge branch 'master' into issue163_purge_old_products
mpejcoch Oct 28, 2021
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
9 changes: 8 additions & 1 deletion cmd/mmsd/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,11 @@ func main() {
Usage: "Specify the port number for the API listening port.",
Value: 8080,
}),
altsrc.NewIntFlag(&cli.IntFlag{
Name: "product-drop-timeout",
Usage: "Specify how many seconds to wait before a product not seen by the system is dropped from the overview. Default is 604800 (7d)",
Value: 604800,
}),
altsrc.NewIntFlag(&cli.IntFlag{
Name: "nats-port",
Usage: "Specify the port number for the NATS listening port.",
Expand Down Expand Up @@ -166,7 +171,7 @@ func main() {
}

templates := server.CreateTemplates()
webService := server.NewService(templates, eventsDB, stateDB, natsURL)
webService := server.NewService(templates, eventsDB, stateDB, natsURL, ctx.Int("product-drop-timeout"))

log.Println("Populating productstatus from the local events database ...")
events, err := webService.GetAllEvents(context.Background())
Expand Down Expand Up @@ -354,6 +359,8 @@ func startEventLoop(webService *server.Service) {
if err := webService.DeleteOldEvents(time.Now().AddDate(0, 0, -3)); err != nil {

@havardf havardf Aug 13, 2021

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Maybe purging events and unused products can be merged into one? So the api from the user of the service do not have to call both?

Like webService.CleanUpEvents, which again deletes old events and removes unused products?

@mpejcoch mpejcoch Aug 13, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The idea was to have separate configuration of how long to keep events and how long to keep products in the monitoring. It might not be necessary when I am thinking about it. Probably it is easier for the user to deal with only 1 parameter.

Purpose of this PR is to address an issue when products keep hanging in the metrics part forever.

We have discussed whether the product status and monitoring should even be part of MMS, there were practical reasons for this as it was easier to maintain on a multi-user machine. We should re-visit this when going over to k8s. It might make sense to split it up and have the functionality as a different component or even a system. The same plan we have with NATS.

@havardf havardf Aug 13, 2021

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Keeping two separate config values for product and old events might make sense, but my comment was more about having just one method call for doing cleanup and this method would handle both product and event cleanup. But if you think its better to keep both config values and cleanup method call separate I won't push it :)

log.Printf("failed to delete old events from events db: %s", err)
}

webService.Productstatus.PurgeOldProducts(604800)
}
}
}()
Expand Down
4 changes: 2 additions & 2 deletions internal/server/api.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ type HTTPServerError struct {
}

// NewService creates a service struct, containing all that is needed for a mmsd server to run.
func NewService(templates *template.Template, eventsDB *sql.DB, stateDB *sql.DB, natsURL string) *Service {
func NewService(templates *template.Template, eventsDB *sql.DB, stateDB *sql.DB, natsURL string, productDropTimeout int) *Service {

@havardf havardf Aug 13, 2021

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I think its time to consider a config struct or something for NewService, as the parameter list to the function is getting pretty long.

@mpejcoch mpejcoch Aug 13, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Agree, I felt I was trying to push it a bit too far yesterday, now I have it confirmed :)

m := NewServiceMetrics(MetricsOpts{})

service := Service{
Expand All @@ -67,7 +67,7 @@ func NewService(templates *template.Template, eventsDB *sql.DB, stateDB *sql.DB,
Router: mux.NewRouter(),
NatsURL: natsURL,
Metrics: m,
Productstatus: NewProductstatus(m),
Productstatus: NewProductstatus(m, productDropTimeout),
}
service.setRoutes()

Expand Down
2 changes: 1 addition & 1 deletion internal/server/cache_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ func NewMockService() (*Service, sqlmock.Sqlmock, error) {
}

templates := CreateTemplates()
webService := NewService(templates, eventsDB, nil, "")
webService := NewService(templates, eventsDB, nil, "", 60)

return webService, mock, nil
}
21 changes: 16 additions & 5 deletions internal/server/productstatus.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,13 +14,15 @@ type Product struct {
}

type Productstatus struct {
Products map[string]Product
GaugeVec *prometheus.GaugeVec
Products map[string]Product
ProductDropTimeout int
GaugeVec *prometheus.GaugeVec
}

func NewProductstatus(m *metrics) *Productstatus {
func NewProductstatus(m *metrics, productDropTimeout int) *Productstatus {
productstatus := Productstatus{
Products: make(map[string]Product),
Products: make(map[string]Product),
ProductDropTimeout: productDropTimeout,
GaugeVec: prometheus.NewGaugeVec(
prometheus.GaugeOpts{
Subsystem: "mmsd",
Expand Down Expand Up @@ -60,7 +62,7 @@ func (p *Productstatus) GetProductDelays(t time.Time) {

func (p *Productstatus) UpdateMetrics() {
for k, v := range p.Products {
diff := time.Now().Sub(v.NextInstanceExpected)
diff := time.Since(v.NextInstanceExpected)
p.GaugeVec.WithLabelValues(k).Set(diff.Seconds())
}
}
Expand All @@ -70,3 +72,12 @@ func (p *Productstatus) Populate(events []*mms.ProductEvent) {
p.PushEvent(*event)
}
}

func (p *Productstatus) PurgeOldProducts(secondsAgo int) {
for k, v := range p.Products {
diff := time.Since(v.NextInstanceExpected)
if diff.Seconds() <= -float64(secondsAgo) {
delete(p.Products, k)
}
}
}
2 changes: 1 addition & 1 deletion internal/server/productstatus_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import (

func TestPushEvent(t *testing.T) {
metrics := NewServiceMetrics(MetricsOpts{})
ps := NewProductstatus(metrics)
ps := NewProductstatus(metrics, 60)

var productEventList [3]mms.ProductEvent

Expand Down