Refactor globalConfiguration / WebProvider #1938

juliens · 2017-08-10T15:16:54Z

split the globalconfiguration to a new package (not in package server)
split traefik configuration and global configuration
webprovider is now in package provider
the run func need a global configuration and not a traefik configuration

nmengin

@juliens Your refacto SGTM.
Only two suggestions.

nmengin · 2017-08-21T09:03:20Z

server/server.go

+			statsRecorder := middlewares.NewStatsRecorder(server.globalConfiguration.Web.Statistics.RecentErrors)
+			server.globalConfiguration.Web.StatsRecorder = statsRecorder
+			serverMiddlewares = append(serverMiddlewares, statsRecorder)
+		}


What is your mind about the deletion of the statsRecorder variable?

if server.globalConfiguration.Web.Statistics != nil { server.globalConfiguration.Web.StatsRecorder = middlewares.NewStatsRecorder(server.globalConfiguration.Web.Statistics.RecentErrors) serverMiddlewares = append(serverMiddlewares, server.globalConfiguration.Web.StatsRecorder) }

nmengin · 2017-08-21T09:04:40Z

server/server.go

+		metricsMiddleware := thoas_stats.New()
+		server.globalConfiguration.Web.MetricsRecorder = metricsMiddleware
+		serverMiddlewares = append(serverMiddlewares, metricsMiddleware)
+		if server.globalConfiguration.Web.Statistics != nil {


What is your mind about the deletion of the metricsMiddleware variable?

if server.globalConfiguration.Web.Statistics != nil { server.globalConfiguration.Web.MetricsRecorder = thoas_stats.New() serverMiddlewares = append(serverMiddlewares, server.globalConfiguration.Web.MetricsRecorder) }

emilevauge

Overall, I find this refactor absolutely amazing 😍
👏 @juliens @ldez !
I have few comments / questions though

emilevauge · 2017-08-24T16:42:18Z

cmd/traefik/traefik.go

 	fmtlog.SetFlags(fmtlog.Lshortfile | fmtlog.LstdFlags)

-	// load global configuration
-	globalConfiguration := traefikConfiguration.GlobalConfiguration
+	http.DefaultTransport.(*http.Transport).MaxIdleConnsPerHost = globalConfiguration.MaxIdleConnsPerHost


Not sure to understand/remember why we apply those parameters to DefaultTransport whereas we used those ones to apply to a specific instance (in server.go createHTTPTransport).

This is actually like code before the introduction of the timeouts PR #1873 and gets reintroduced here, which it shouldn't. Can you align your version please.

emilevauge · 2017-08-24T16:47:04Z

provider/web/web.go

+	Debug                 bool
+	CurrentConfigurations *safe.Safe
+	MetricsRecorder       *thoas_stats.Stats
+	StatsRecorder         *middlewares.StatsRecorder


Is it intended (MetricsRecorder & StatsRecorder)?

Imho the naming is confusing. Can you rename MetricsRecorder with Stats to make the distinction with the existing Metrics implementation clear?

To be sure, fields like StatsRecorder can not be configured as part of the Web provider now?

emilevauge · 2017-08-24T16:49:47Z

provider/web/web.go

-	server     *Server
-	Auth       *types.Auth
+// Provider is a provider.Provider implementation that provides the UI
+type Provider struct {


Shouldn't this struct embed a provider.BaseProvider like any other provider ?

emilevauge · 2017-08-24T17:58:41Z

server/server.go

-	if server.globalConfiguration.Web != nil && server.globalConfiguration.Web.Statistics != nil {
-		statsRecorder = middlewares.NewStatsRecorder(server.globalConfiguration.Web.Statistics.RecentErrors)
-		serverMiddlewares = append(serverMiddlewares, statsRecorder)
+	if server.globalConfiguration.Web != nil {


I wonder if these changes are compliant with the metrics refactor made in #1968

ping @marco-jantke

AFAICS it doesn't interfere with the metrics implementation but the naming confuses at the moment. Its a good step by the way to get rid of the global stats variable. Anyhow MetricsRecorder is a confusing naming. I would stick with Stats.

dtomcej · 2017-08-25T03:20:10Z

This may seem like a silly question, but if we are migrating the web provider to its own middleware, can we use this opportunity to rename it to something like 'api', or 'rest' or some sort of API-like provider name?

It can be very confusing for people to have the API and the Admin panel be named "web"

timoreimann · 2017-08-25T07:50:11Z

+1 to what @dtomcej said.

ldez · 2017-08-25T07:54:33Z

It's not only a rest api but it's also an endpoint for the webui. A good name is hard to find.

timoreimann · 2017-08-25T07:57:44Z

True...

m3co-code

I have the feeling the package naming is quite verbose, as one example configuration.TraeifkConfiguration. Should we maybe use only and consistently config like config.TraefikConfig?

m3co-code · 2017-08-25T12:18:23Z

cmd/traefik/traefik.go

 	fmtlog.SetFlags(fmtlog.Lshortfile | fmtlog.LstdFlags)

-	// load global configuration
-	globalConfiguration := traefikConfiguration.GlobalConfiguration
+	http.DefaultTransport.(*http.Transport).MaxIdleConnsPerHost = globalConfiguration.MaxIdleConnsPerHost


This is actually like code before the introduction of the timeouts PR #1873 and gets reintroduced here, which it shouldn't. Can you align your version please.

m3co-code · 2017-08-25T12:18:58Z

provider/web/web.go

+	Debug                 bool
+	CurrentConfigurations *safe.Safe
+	MetricsRecorder       *thoas_stats.Stats
+	StatsRecorder         *middlewares.StatsRecorder


Imho the naming is confusing. Can you rename MetricsRecorder with Stats to make the distinction with the existing Metrics implementation clear?

To be sure, fields like StatsRecorder can not be configured as part of the Web provider now?

m3co-code · 2017-08-25T12:20:47Z

server/server.go

-	if server.globalConfiguration.Web != nil && server.globalConfiguration.Web.Statistics != nil {
-		statsRecorder = middlewares.NewStatsRecorder(server.globalConfiguration.Web.Statistics.RecentErrors)
-		serverMiddlewares = append(serverMiddlewares, statsRecorder)
+	if server.globalConfiguration.Web != nil {


AFAICS it doesn't interfere with the metrics implementation but the naming confuses at the moment. Its a good step by the way to get rid of the global stats variable. Anyhow MetricsRecorder is a confusing naming. I would stick with Stats.

ldez

LGTM

nmengin

LGTM 👏

emilevauge · 2017-08-25T13:23:23Z

I have the feeling the package naming is quite verbose, as one example configuration.TraeifkConfiguration. Should we maybe use only and consistently config like config.TraefikConfig?

Hey @marco-jantke, we all use automatic completion today, so I think we can live we "long" package names ;)

emilevauge

🎉
LGTM

m3co-code

LGTM 🎉

- Traefik configuration is in main - Metrics and statsRecord are not global vars anymore - run use GlobalConfiguration - fix log rotation test to adapt to he new number of lines due to the refactoring

juliens added WIP kind/enhancement a new or improved feature. labels Aug 10, 2017

ldez changed the title ~~Refacto globalConfiguration / WebProvider~~ Refactor globalConfiguration / WebProvider Aug 10, 2017

ldez added this to the 1.4 milestone Aug 11, 2017

juliens force-pushed the refacto-package-configuration branch from 94d42ec to dc348d3 Compare August 12, 2017 14:13

ldez added WIP and removed WIP labels Aug 15, 2017

juliens force-pushed the refacto-package-configuration branch from dc348d3 to 931ad6f Compare August 15, 2017 21:32

ldez added status/2-needs-review and removed WIP labels Aug 17, 2017

nmengin force-pushed the refacto-package-configuration branch from 931ad6f to a568d2d Compare August 18, 2017 16:04

ldez added the size/L label Aug 19, 2017

nmengin reviewed Aug 21, 2017

View reviewed changes

ldez force-pushed the refacto-package-configuration branch 3 times, most recently from e046ae8 to 6674e82 Compare August 24, 2017 14:27

emilevauge reviewed Aug 24, 2017

View reviewed changes

m3co-code suggested changes Aug 25, 2017

View reviewed changes

traefiker added the contributor/waiting-for-corrections label Aug 25, 2017

ldez force-pushed the refacto-package-configuration branch 3 times, most recently from 0ccaf5e to 1bf033a Compare August 25, 2017 13:18

ldez removed the contributor/waiting-for-corrections label Aug 25, 2017

ldez approved these changes Aug 25, 2017

View reviewed changes

nmengin approved these changes Aug 25, 2017

View reviewed changes

emilevauge approved these changes Aug 25, 2017

View reviewed changes

m3co-code approved these changes Aug 25, 2017

View reviewed changes

refactor: configuration in dedicated package

fa8396f

- Traefik configuration is in main - Metrics and statsRecord are not global vars anymore - run use GlobalConfiguration - fix log rotation test to adapt to he new number of lines due to the refactoring

ldez force-pushed the refacto-package-configuration branch from 1bf033a to fa8396f Compare August 25, 2017 13:43

ldez added status/3-needs-merge and removed status/2-needs-review labels Aug 25, 2017

traefiker merged commit e0af17a into traefik:master Aug 25, 2017

traefiker removed the status/3-needs-merge label Aug 25, 2017

juliens deleted the refacto-package-configuration branch September 13, 2017 13:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor globalConfiguration / WebProvider #1938

Refactor globalConfiguration / WebProvider #1938

juliens commented Aug 10, 2017

nmengin left a comment •

edited

Loading

nmengin Aug 21, 2017 •

edited

Loading

juliens Aug 22, 2017

nmengin Aug 21, 2017 •

edited

Loading

juliens Aug 22, 2017

emilevauge left a comment

emilevauge Aug 24, 2017

m3co-code Aug 25, 2017

emilevauge Aug 24, 2017

m3co-code Aug 25, 2017 •

edited

Loading

emilevauge Aug 24, 2017

emilevauge Aug 24, 2017

ldez Aug 24, 2017

m3co-code Aug 25, 2017

dtomcej commented Aug 25, 2017

timoreimann commented Aug 25, 2017

ldez commented Aug 25, 2017

timoreimann commented Aug 25, 2017

m3co-code left a comment

m3co-code Aug 25, 2017

m3co-code Aug 25, 2017 •

edited

Loading

m3co-code Aug 25, 2017

ldez left a comment

nmengin left a comment

emilevauge commented Aug 25, 2017

emilevauge left a comment

m3co-code left a comment

Refactor globalConfiguration / WebProvider #1938

Refactor globalConfiguration / WebProvider #1938

Conversation

juliens commented Aug 10, 2017

nmengin left a comment • edited Loading

Choose a reason for hiding this comment

nmengin Aug 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nmengin Aug 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emilevauge 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

m3co-code Aug 25, 2017 • edited Loading

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

dtomcej commented Aug 25, 2017

timoreimann commented Aug 25, 2017

ldez commented Aug 25, 2017

timoreimann commented Aug 25, 2017

m3co-code left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

m3co-code Aug 25, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ldez left a comment

Choose a reason for hiding this comment

nmengin left a comment

Choose a reason for hiding this comment

emilevauge commented Aug 25, 2017

emilevauge left a comment

Choose a reason for hiding this comment

m3co-code left a comment

Choose a reason for hiding this comment

nmengin left a comment •

edited

Loading

nmengin Aug 21, 2017 •

edited

Loading

nmengin Aug 21, 2017 •

edited

Loading

m3co-code Aug 25, 2017 •

edited

Loading

m3co-code Aug 25, 2017 •

edited

Loading