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(config): Make MaximumNumberOfResults and MaximumNumberOfEvents configurable #476

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

skhokhlov
Copy link
Contributor

@skhokhlov skhokhlov commented May 12, 2023

Summary

#466

Checklist

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

@skhokhlov skhokhlov changed the title feat(config): Make MaximumNumberOfResults and MaximumNumberOfEvents c… feat(config): Make MaximumNumberOfResults and MaximumNumberOfEvents configurable May 12, 2023
config/config.go Outdated Show resolved Hide resolved
@BassSingh
Copy link

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

Copy link
Owner

@TwiN TwiN left a comment

Choose a reason for hiding this comment

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

Awesome work!

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

README.md Outdated
Comment on lines 305 to 306
| `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` |
Copy link
Owner

Choose a reason for hiding this comment

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

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

config/ui/ui.go Outdated
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
Copy link
Owner

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Owner

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Comment on lines 163 to 166
Storage: &storage.Config{
MaximumNumberOfResults: storage.DefaultMaximumNumberOfResults,
MaximumNumberOfEvents: storage.DefaultMaximumNumberOfEvents,
},
Copy link
Owner

Choose a reason for hiding this comment

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

This should not be necessary.

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

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:

func (c *Config) ValidateAndSetDefaults() error {
(the latter seems to have already been done)

Copy link
Owner

Choose a reason for hiding this comment

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

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 🤔

Copy link
Contributor Author

@skhokhlov skhokhlov May 19, 2023

Choose a reason for hiding this comment

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

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?

)

func extractPageAndPageSizeFromRequest(r *http.Request) (page int, pageSize int) {
func extractPageAndPageSizeFromRequest(r *http.Request, cfg *config.Config) (page int, pageSize int) {
Copy link
Owner

Choose a reason for hiding this comment

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

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

Comment on lines 34 to 38
// 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"`
Copy link
Owner

Choose a reason for hiding this comment

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

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

Comment on lines 5 to 6
"github.com/TwiN/gatus/v5/config"
"github.com/TwiN/gatus/v5/storage"
Copy link
Owner

Choose a reason for hiding this comment

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

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"
)

Comment on lines 63 to 66
// 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
Copy link
Owner

Choose a reason for hiding this comment

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

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.

@skhokhlov skhokhlov requested a review from TwiN May 19, 2023 10:26
@skhokhlov skhokhlov force-pushed the config-maximum-number-results-events branch from 72e16e8 to bacf86d Compare May 19, 2023 11:02
config/ui/ui.go Outdated
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
Copy link
Owner

Choose a reason for hiding this comment

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

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.

Comment on lines 58 to 66
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)
Copy link
Owner

Choose a reason for hiding this comment

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

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.

Comment on lines 52 to 57
if c.MaximumNumberOfResults == 0 {
c.MaximumNumberOfResults = DefaultMaximumNumberOfResults
}
if c.MaximumNumberOfEvents == 0 {
c.MaximumNumberOfEvents = DefaultMaximumNumberOfEvents
}
Copy link
Owner

Choose a reason for hiding this comment

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

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
}

Comment on lines 393 to 396
_, _ = store.db.Exec("DROP TABLE endpoints")
_, _ = store.db.Exec("drop table endpoints")
Copy link
Owner

Choose a reason for hiding this comment

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

Can you revert this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, it was auto formatting in my IDE

Comment on lines 422 to 425
_, _ = store.db.Exec("DROP TABLE endpoint_events")
_, _ = store.db.Exec("drop table endpoint_events")
Copy link
Owner

Choose a reason for hiding this comment

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

Can you revert this change?

Comment on lines 438 to 441
_, _ = store.db.Exec("DROP TABLE endpoint_results")
_, _ = store.db.Exec("drop table endpoint_results")
Copy link
Owner

Choose a reason for hiding this comment

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

Can you revert this change?

@@ -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")
Copy link
Owner

Choose a reason for hiding this comment

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

Can you revert this change?

Comment on lines 454 to 457
_, _ = store.db.Exec("DROP TABLE endpoint_result_conditions")
_, _ = store.db.Exec("drop table endpoint_result_conditions")
Copy link
Owner

Choose a reason for hiding this comment

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

Can you revert this change?

@skhokhlov skhokhlov requested a review from TwiN May 22, 2023 11:44
@skhokhlov
Copy link
Contributor Author

@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
Copy link

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
Copy link
Contributor

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

@TwiN TwiN mentioned this pull request Aug 8, 2023
…er-results-events

# Conflicts:
#	storage/store/store_test.go
#	web/static/css/app.css
@skhokhlov skhokhlov force-pushed the config-maximum-number-results-events branch from 489be65 to 04b33ec Compare August 11, 2023 12:57
@emouawad
Copy link

Reviving.. is this still planned?

@jonisapp
Copy link

Any update on this ? Gatus does so many things, so it's a bit disappointing not to be able to configure the data retention period...

@TwiN
Copy link
Owner

TwiN commented Sep 23, 2024

Looks like there's a couple of merge conflicts due to how long it has been.

I've always been holding back about this, mostly because the way the information is displayed would provide a bad user experience.

Right now, you have to go one page at a time on the dashboard, which is 20 results each. If there were 1000 results, you'd have to click 50 times to get to the last page.

Either way, since this has been requested so many times, we may as well give it a shot.

P.S. due to how many code changes have been made, it may not be possible to easily resolve the conflicts in this PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants