feat(config): Make MaximumNumberOfResults and MaximumNumberOfEvents configurable #476

skhokhlov · 2023-05-12T08:40:01Z

Summary

Checklist

Tested and/or added tests to validate that the changes work as intended, if applicable.
Updated documentation in README.md, if applicable.

config/config.go

BassSingh · 2023-05-18T12:27:17Z

@TwiN When you get a chance could you review this please? This looks so awesome and I think it will really benefit Gatus!

TwiN

Awesome work!

There's still a few more changes I want to look closer at, but let's start with this.

TwiN · 2023-05-19T02:09:14Z

README.md

+| `storage.maximumNumberOfResults` | The maximum number of results that an endpoint can have                                                                                            | `100`      |
+| `storage.maximumNumberOfEvents`  | The maximum number of events that an endpoint can have                                                                                             | `50`       |


For consistency, maximumNumberOfResults and maximumNumberOfEvents should be maximum-number-of-results and maximum-number-of-events respectively.

TwiN · 2023-05-19T02:19:51Z

config/ui/ui.go

+	Logo                   string   `yaml:"logo,omitempty"`        // Logo to display on the page
+	Link                   string   `yaml:"link,omitempty"`        // Link to open when clicking on the logo
+	Buttons                []Button `yaml:"buttons,omitempty"`     // Buttons to display below the header
+	MaximumNumberOfResults int      // MaximumNumberOfResults to display on the page


I'd rather not make this configurable at the UI level & let it be handled by the storage configuration. Since the UI supports pagination, we don't have to worry about this at the UI.

Yes, I'm agree, that's why I did this https://github.com/TwiN/gatus/pull/476/files#diff-fe44f09c4d5977b5f5eaea29170b6a0748819c9d02271746a20d81a5f3efca17R262 It's a workaround to pass this value to ui, we need to to calculate number of pages, because it's hardcoded in the ui https://github.com/TwiN/gatus/pull/476/files#diff-1c806791cec1d6fff630b04440dfb90fff5a34ef3a17cadadfbeafa2c26dcfe2L4

Ah that makes sense! Could you perhaps add a newline above MaximumNumberOfResults to separate it from the other parameters?

Also perhaps updating the documentation to make it clear why this field is different from the others (i.e. it's not configurable because we're passing it from the UI?)

Alternatively, I wonder if we should just not pass this at all & when a user tries to go to a page with no results, just mentioned that there's no results.

Alternatively, I wonder if we should just not pass this at all & when a user tries to go to a page with no results, just mentioned that there's no results.

I think this way is really better, but I'm afraid that I'm not so familiar with vue.js to implement this change

TwiN · 2023-05-19T02:24:54Z

controller/handler/endpoint_status_test.go

+		Storage: &storage.Config{
+			MaximumNumberOfResults: storage.DefaultMaximumNumberOfResults,
+			MaximumNumberOfEvents:  storage.DefaultMaximumNumberOfEvents,
+		},


This should not be necessary.

For test cases, the default values should be set in storage's Initialize function, which is called by `

gatus/storage/store/store.go

Lines 75 to 85 in 3350e81

func Get() Store {

if !initialized {

// This only happens in tests

log.Println("[store][Get] Provider requested before it was initialized, automatically initializing")

err := Initialize(nil)

if err != nil {

panic("failed to automatically initialize store: " + err.Error())

}

}

return store

}

for tests.

As for how it should be handled for non-test cases:

gatus/storage/config.go

Line 31 in 3350e81

func (c *Config) ValidateAndSetDefaults() error {

(the latter seems to have already been done)

After reviewing changes further down, it seems like you've also done the change in storage.Initialize, so I'm not sure defining a storage config here is even necessary 🤔

I cannot just rely on Initialize function because I need the config initialised before store.Get() call here https://github.com/TwiN/gatus/pull/476/files#diff-8cfab31fa8754c8f5da1e4f0222ea131e70dead2ebfb3a77b14950919627026dR34 and here https://github.com/TwiN/gatus/pull/476/files#diff-8cfab31fa8754c8f5da1e4f0222ea131e70dead2ebfb3a77b14950919627026dR103

We can call store.Get() first before extracting pages and sized from request and checking cache, but does it worth it?

TwiN · 2023-05-19T02:26:32Z

controller/handler/util.go

 )

-func extractPageAndPageSizeFromRequest(r *http.Request) (page int, pageSize int) {
+func extractPageAndPageSizeFromRequest(r *http.Request, cfg *config.Config) (page int, pageSize int) {


It feels kind of overkill to pass the encore configuration when all you need is the MaximumNumberOfResults

TwiN · 2023-05-19T02:27:43Z

storage/config.go

+	// MaximumNumberOfResults is the maximum number of results that an endpoint can have
+	MaximumNumberOfResults int `yaml:"maximumNumberOfResults,omitempty"`
+
+	// MaximumNumberOfEvents is the maximum number of events that an endpoint can have
+	MaximumNumberOfEvents int `yaml:"maximumNumberOfEvents,omitempty"`


As mentioned in another review comment, maximumNumberOfResults and maximumNumberOfEvents should be maximum-number-of-results and maximum-number-of-events respectively.

TwiN · 2023-05-19T02:29:38Z

controller/handler/util_test.go

+	"github.com/TwiN/gatus/v5/config"
+	"github.com/TwiN/gatus/v5/storage"


The imports should be split in two groups: imports from the standard library, and external imports.

In other words, this should be

import ( "fmt" "net/http" "testing" "github.com/TwiN/gatus/v5/config" "github.com/TwiN/gatus/v5/storage" )

TwiN · 2023-05-19T02:40:11Z

storage/store/sql/sql.go

+	// eventsCleanUpThreshold is a maximum number of events before triggering a clean up
+	eventsCleanUpThreshold int
+	// resultsCleanUpThreshold is a maximum number of results before triggering a clean up
+	resultsCleanUpThreshold int


Instead of defining eventsCleanUpThreshold and resultsCleanUpThreshold in the store, we should create constants named something like eventsAboveMaximumCleanUpThreshold and resultsAboveMaximumCleanUpThreshold & set both of these to 10.

Then we can just compute the threshold based on store.maximumNumberOfEvents + eventsAboveMaximumCleanUpThreshold and store.maximumNumberOfResults + resultsAboveMaximumCleanUpThreshold.

TwiN · 2023-05-21T16:33:39Z

config/ui/ui.go

+	Logo                   string   `yaml:"logo,omitempty"`        // Logo to display on the page
+	Link                   string   `yaml:"link,omitempty"`        // Link to open when clicking on the logo
+	Buttons                []Button `yaml:"buttons,omitempty"`     // Buttons to display below the header
+	MaximumNumberOfResults int      // MaximumNumberOfResults to display on the page


Ah that makes sense! Could you perhaps add a newline above MaximumNumberOfResults to separate it from the other parameters?

Also perhaps updating the documentation to make it clear why this field is different from the others (i.e. it's not configurable because we're passing it from the UI?)

Alternatively, I wonder if we should just not pass this at all & when a user tries to go to a page with no results, just mentioned that there's no results.

TwiN · 2023-05-21T16:41:33Z

controller/handler/util_test.go

+	storageCfg := &storage.Config{}
+	storageCfg.ValidateAndSetDefaults()
+	cfg := &config.Config{
+		Storage: storageCfg,
+	}
 	for _, scenario := range scenarios {
 		t.Run("page-"+scenario.Page+"-pageSize-"+scenario.PageSize, func(t *testing.T) {
 			request, _ := http.NewRequest("GET", fmt.Sprintf("/api/v1/statuses?page=%s&pageSize=%s", scenario.Page, scenario.PageSize), http.NoBody)
-			actualPage, actualPageSize := extractPageAndPageSizeFromRequest(request)
+			actualPage, actualPageSize := extractPageAndPageSizeFromRequest(request, cfg.Storage.MaximumNumberOfResults)


Is there really a need to initialize a storage config here since we're just passing cfg.Storage.MaximumNumberOfResults?

I think we'd be better off removing

storageCfg := &storage.Config{} storageCfg.ValidateAndSetDefaults() cfg := &config.Config{ Storage: storageCfg, }

adding MaximumNumberOfResults to the scenario struct definition and just doing

actualPage, actualPageSize := extractPageAndPageSizeFromRequest(request, scenario.MaximumNumberOfResults)

That way we can properly test the behavior of extractPageAndPageSizeFromRequest with different MaximumNumberOfResults values.

TwiN · 2023-05-21T16:42:15Z

storage/config.go

+	if c.MaximumNumberOfResults == 0 {
+		c.MaximumNumberOfResults = DefaultMaximumNumberOfResults
+	}
+	if c.MaximumNumberOfEvents == 0 {
+		c.MaximumNumberOfEvents = DefaultMaximumNumberOfEvents
+	}


Suggested change

if c.MaximumNumberOfResults == 0 {

c.MaximumNumberOfResults = DefaultMaximumNumberOfResults

}

if c.MaximumNumberOfEvents == 0 {

c.MaximumNumberOfEvents = DefaultMaximumNumberOfEvents

}

if c.MaximumNumberOfResults <= 0 {

c.MaximumNumberOfResults = DefaultMaximumNumberOfResults

}

if c.MaximumNumberOfEvents <= 0 {

c.MaximumNumberOfEvents = DefaultMaximumNumberOfEvents

}

TwiN · 2023-05-21T16:46:08Z

storage/store/sql/sql_test.go

-	_, _ = store.db.Exec("DROP TABLE endpoints")
+	_, _ = store.db.Exec("drop table endpoints")


Can you revert this change?

Sorry, it was auto formatting in my IDE

TwiN · 2023-05-21T16:46:20Z

storage/store/sql/sql_test.go

-	_, _ = store.db.Exec("DROP TABLE endpoint_events")
+	_, _ = store.db.Exec("drop table endpoint_events")


Can you revert this change?

TwiN · 2023-05-21T16:46:24Z

storage/store/sql/sql_test.go

-	_, _ = store.db.Exec("DROP TABLE endpoint_results")
+	_, _ = store.db.Exec("drop table endpoint_results")


Can you revert this change?

TwiN · 2023-05-21T16:46:28Z

storage/store/sql/sql_test.go

@@ -464,7 +467,7 @@ func TestStore_BrokenSchema(t *testing.T) {
 		t.Fatal("expected no error, got", err.Error())
 	}
 	// Break
-	_, _ = store.db.Exec("DROP TABLE endpoint_uptimes")
+	_, _ = store.db.Exec("drop table endpoint_uptimes")


Can you revert this change?

TwiN · 2023-05-21T16:46:32Z

storage/store/sql/sql_test.go

-	_, _ = store.db.Exec("DROP TABLE endpoint_result_conditions")
+	_, _ = store.db.Exec("drop table endpoint_result_conditions")


Can you revert this change?

skhokhlov · 2023-06-02T15:08:12Z

@TwiN how does the process of PR work in this project? Who should resolve conversations, the one asking or the one answering? You don't have contributing guidelines in this repo so it's not clear.

BassSingh · 2023-06-27T10:43:22Z

Hey lovely @TwiN is there any chance you could review this sir? It would be amazing if we could get this feature as we absolutely love Gatus!!! <3

heitorPB · 2023-07-25T19:49:04Z

What exactly is an event in this context? What's the difference to a result?

…onfigurable

…er-results-events # Conflicts: # storage/store/store_test.go # web/static/css/app.css

Signed-off-by: Sergey Khokhlov <sergey.khokhlov@citi.com>

emouawad · 2024-05-30T02:00:02Z

Reviving.. is this still planned?

skhokhlov changed the title ~~feat(config): Make MaximumNumberOfResults and MaximumNumberOfEvents c…~~ feat(config): Make MaximumNumberOfResults and MaximumNumberOfEvents configurable May 12, 2023

skhokhlov commented May 12, 2023

View reviewed changes

config/config.go Outdated Show resolved Hide resolved

TwiN requested changes May 19, 2023

View reviewed changes

skhokhlov requested a review from TwiN May 19, 2023 10:26

skhokhlov force-pushed the config-maximum-number-results-events branch from 72e16e8 to bacf86d Compare May 19, 2023 11:02

TwiN requested changes May 21, 2023

View reviewed changes

skhokhlov requested a review from TwiN May 22, 2023 11:44

TwiN mentioned this pull request Aug 8, 2023

[Question] Data retention #540

Closed

skhokhlov added 2 commits August 11, 2023 14:17

feat(config): Make MaximumNumberOfResults and MaximumNumberOfEvents c…

e63814c

…onfigurable

Merge remote-tracking branch 'origin/master' into config-maximum-numb…

04b33ec

…er-results-events # Conflicts: # storage/store/store_test.go # web/static/css/app.css

skhokhlov force-pushed the config-maximum-number-results-events branch from 489be65 to 04b33ec Compare August 11, 2023 12:57

feat(config): Fix store_test after merge

a49342c

Signed-off-by: Sergey Khokhlov <sergey.khokhlov@citi.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(config): Make MaximumNumberOfResults and MaximumNumberOfEvents configurable #476

feat(config): Make MaximumNumberOfResults and MaximumNumberOfEvents configurable #476

skhokhlov commented May 12, 2023 •

edited

BassSingh commented May 18, 2023

TwiN left a comment

TwiN May 19, 2023

TwiN May 19, 2023

skhokhlov May 19, 2023

TwiN May 21, 2023

skhokhlov May 22, 2023

TwiN May 19, 2023

TwiN May 19, 2023

skhokhlov May 19, 2023 •

edited

TwiN May 19, 2023

TwiN May 19, 2023

TwiN May 19, 2023

TwiN May 19, 2023

TwiN May 21, 2023

TwiN May 21, 2023

TwiN May 21, 2023

TwiN May 21, 2023

skhokhlov May 22, 2023

TwiN May 21, 2023

TwiN May 21, 2023

TwiN May 21, 2023

TwiN May 21, 2023

skhokhlov commented Jun 2, 2023

BassSingh commented Jun 27, 2023

heitorPB commented Jul 25, 2023

emouawad commented May 30, 2024

		\| `storage.maximumNumberOfResults` \| The maximum number of results that an endpoint can have \| `100` \|
		\| `storage.maximumNumberOfEvents` \| The maximum number of events that an endpoint can have \| `50` \|

	func Get() Store {
	if !initialized {
	// This only happens in tests
	log.Println("[store][Get] Provider requested before it was initialized, automatically initializing")
	err := Initialize(nil)
	if err != nil {
	panic("failed to automatically initialize store: " + err.Error())
	}
	}
	return store
	}

		"github.com/TwiN/gatus/v5/config"
		"github.com/TwiN/gatus/v5/storage"

		_, _ = store.db.Exec("DROP TABLE endpoints")
		_, _ = store.db.Exec("drop table endpoints")

feat(config): Make MaximumNumberOfResults and MaximumNumberOfEvents configurable #476

Are you sure you want to change the base?

feat(config): Make MaximumNumberOfResults and MaximumNumberOfEvents configurable #476

Conversation

skhokhlov commented May 12, 2023 • edited

Summary

Checklist

BassSingh commented May 18, 2023

TwiN left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skhokhlov May 19, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skhokhlov commented Jun 2, 2023

BassSingh commented Jun 27, 2023

heitorPB commented Jul 25, 2023

emouawad commented May 30, 2024

skhokhlov commented May 12, 2023 •

edited

skhokhlov May 19, 2023 •

edited