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

📊 add refresh period to monitor mw #1898

Merged
merged 1 commit into from May 16, 2022
Merged

Conversation

jfcg
Copy link
Contributor

@jfcg jfcg commented May 7, 2022

No description provided.

@ReneWerner87
Copy link
Member

ReneWerner87 commented May 9, 2022

@jfcg thx
i don't see any tests for the newly generated code, please add some that reflect what you expect to happen

@jfcg
Copy link
Contributor Author

jfcg commented May 9, 2022

extended Test_Monitor_Html to also check timeout line.

Copy link
Member

@ReneWerner87 ReneWerner87 left a comment

.

ConfigDefault.Refresh = minRefresh
}
// update default index with new default title/refresh
ConfigDefault.index = newIndex(ConfigDefault.Title, ConfigDefault.Refresh)
Copy link
Member

@ReneWerner87 ReneWerner87 May 12, 2022

Choose a reason for hiding this comment

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

why don't we do this below when refresh and title has been reset or contains the default values ?

Copy link
Contributor Author

@jfcg jfcg May 12, 2022

Choose a reason for hiding this comment

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

If a user changes default config only, like:

monitor.ConfigDefault.Title = "New Title"
app.Get("/metrics", monitor.New())

then these lines will return default config directly:

	// Return default config if nothing provided
	if len(config) < 1 {
		return ConfigDefault
	}

so default config must be prepared beforehand 😉

Copy link
Member

@ReneWerner87 ReneWerner87 May 14, 2022

Choose a reason for hiding this comment

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

Why should the users do that?
Then they have not understood the concept or we should not expose the default config

Copy link
Contributor Author

@jfcg jfcg May 14, 2022

Choose a reason for hiding this comment

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

If we can unexport default config, that would be easier but I think that decision should be considered for the whole project, not just for monitor mw.

Note that if something is exported to users, then it is part of the API and meant to be usable/modifiable.

Copy link
Contributor Author

@jfcg jfcg May 14, 2022

Choose a reason for hiding this comment

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

Though there does not seem to be many pablic uses of monitor.DefaultConfig, as per Github search.

@@ -38,6 +39,9 @@ func Test_Monitor_Html(t *testing.T) {
b, err := ioutil.ReadAll(resp.Body)
utils.AssertEqual(t, nil, err)
utils.AssertEqual(t, true, bytes.Contains(b, []byte("<title>"+defaultTitle+"</title>")))
timeoutLine := fmt.Sprintf("setTimeout(fetchJSON, %d)",
Copy link
Member

@ReneWerner87 ReneWerner87 May 12, 2022

Choose a reason for hiding this comment

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

please also create a test where the setting of the configuration and its modification is checked

Copy link
Contributor Author

@jfcg jfcg May 13, 2022

Choose a reason for hiding this comment

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

extended Test_Monitor_Html to also check custom config.

Copy link
Member

@ReneWerner87 ReneWerner87 left a comment

pls check my review comments

@ReneWerner87 ReneWerner87 merged commit 0fdd7df into gofiber:master May 16, 2022
16 checks passed
trim21 pushed a commit to trim21/fiber that referenced this issue Aug 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants