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.

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

Jump to bottom

Allow to disable NGINX metrics #3512

Merged
merged 1 commit into from
Dec 5, 2018
Merged

Allow to disable NGINX metrics #3512

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
4 changes: 4 additions & 0 deletions cmd/nginx/flags.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,6 +147,9 @@ Requires the update-status parameter.`)
`Dynamically update SSL certificates instead of reloading NGINX.
Feature backed by OpenResty Lua libraries. Requires that OCSP stapling is not enabled`)

enableMetrics = flags.Bool("enable-metrics", true,
`Enables the collection of NGINX metrics`)

httpPort = flags.Int("http-port", 80, `Port to use for servicing HTTP traffic.`)
httpsPort = flags.Int("https-port", 443, `Port to use for servicing HTTPS traffic.`)
statusPort = flags.Int("status-port", 18080, `Port to use for exposing NGINX status pages.`)
Expand Down Expand Up @@ -223,6 +226,7 @@ Feature backed by OpenResty Lua libraries. Requires that OCSP stapling is not en
UpdateStatus: *updateStatus,
ElectionID: *electionID,
EnableProfiling: *profiling,
EnableMetrics: *enableMetrics,
EnableSSLPassthrough: *enableSSLPassthrough,
EnableSSLChainCompletion: *enableSSLChainCompletion,
ResyncPeriod: *resyncPeriod,
Expand Down
9 changes: 6 additions & 3 deletions cmd/nginx/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,9 +128,12 @@ func main() {
ReportErrors: true,
}))

mc, err := metric.NewCollector(conf.ListenPorts.Status, reg)
if err != nil {
glog.Fatalf("Error creating prometheus collector: %v", err)
mc := metric.NewDummyCollector()
if conf.EnableMetrics {
mc, err = metric.NewCollector(conf.ListenPorts.Status, reg)
if err != nil {
glog.Fatalf("Error creating prometheus collector: %v", err)
}
}
mc.Start()

Expand Down
1 change: 1 addition & 0 deletions internal/ingress/controller/config/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -715,6 +715,7 @@ type TemplateConfig struct {
ListenPorts *ListenPorts
PublishService *apiv1.Service
DynamicCertificatesEnabled bool
EnableMetrics bool
}

// ListenPorts describe the ports required to run the
Expand Down
2 changes: 2 additions & 0 deletions internal/ingress/controller/controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,8 @@ type Configuration struct {

EnableProfiling bool

EnableMetrics bool

EnableSSLChainCompletion bool

FakeCertificatePath string
Expand Down
1 change: 1 addition & 0 deletions internal/ingress/controller/nginx.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -602,6 +602,7 @@ func (n *NGINXController) OnUpdate(ingressCfg ingress.Configuration) error {
ListenPorts: n.cfg.ListenPorts,
PublishService: n.GetPublishService(),
DynamicCertificatesEnabled: n.cfg.DynamicCertificatesEnabled,
EnableMetrics: n.cfg.EnableMetrics,
}

tc.Cfg.Checksum = ingressCfg.ConfigurationChecksum
Expand Down
4 changes: 3 additions & 1 deletion internal/ingress/controller/template/template.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -914,13 +914,15 @@ func proxySetHeader(loc interface{}) string {

// buildCustomErrorDeps is a utility function returning a struct wrapper with
// the data required to build the 'CUSTOM_ERRORS' template
func buildCustomErrorDeps(proxySetHeaders map[string]string, errorCodes []int) interface{} {
func buildCustomErrorDeps(proxySetHeaders map[string]string, errorCodes []int, enableMetrics bool) interface{} {
return struct {
ProxySetHeaders map[string]string
ErrorCodes []int
EnableMetrics bool
}{
ProxySetHeaders: proxySetHeaders,
ErrorCodes: errorCodes,
EnableMetrics: enableMetrics,
}
}

Expand Down
2 changes: 1 addition & 1 deletion internal/ingress/controller/template/template_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -826,7 +826,7 @@ func TestEscapeLiteralDollar(t *testing.T) {
}
}

func Test_opentracingPropagateContext(t *testing.T) {
func TestOpentracingPropagateContext(t *testing.T) {
tests := map[interface{}]string{
&ingress.Location{BackendProtocol: "HTTP"}: "opentracing_propagate_context",
&ingress.Location{BackendProtocol: "HTTPS"}: "opentracing_propagate_context",
Expand Down
13 changes: 12 additions & 1 deletion internal/ingress/metric/dummy.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,15 @@ limitations under the License.

package metric

import "k8s.io/ingress-nginx/internal/ingress"
import (
Copy link
Member

Choose a reason for hiding this comment

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

things in this file weren't being used anywhere before right?

Copy link
Member Author

Choose a reason for hiding this comment

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

correct

"k8s.io/apimachinery/pkg/util/sets"
"k8s.io/ingress-nginx/internal/ingress"
)

// NewDummyCollector returns a dummy metric collector
func NewDummyCollector() Collector {
return &DummyCollector{}
}

// DummyCollector dummy implementation for mocks in tests
type DummyCollector struct{}
Expand All @@ -41,3 +49,6 @@ func (dc DummyCollector) Stop() {}

// SetSSLExpireTime ...
func (dc DummyCollector) SetSSLExpireTime([]*ingress.Server) {}

// SetHosts ...
func (dc DummyCollector) SetHosts(hosts sets.String) {}
15 changes: 12 additions & 3 deletions rootfs/etc/nginx/template/nginx.tmpl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,12 +78,14 @@ http {
balancer = res
end

{{ if $all.EnableMetrics }}
ok, res = pcall(require, "monitor")
if not ok then
error("require failed: " .. tostring(res))
else
monitor = res
end
{{ end }}

{{ if $all.DynamicCertificatesEnabled }}
ok, res = pcall(require, "certificate")
Expand All @@ -97,7 +99,9 @@ http {

init_worker_by_lua_block {
balancer.init_worker()
{{ if $all.EnableMetrics }}
monitor.init_worker()
{{ end }}
}

{{/* Enable the real_ip module only if we use either X-Forwarded headers or Proxy Protocol. */}}
Expand Down Expand Up @@ -576,7 +580,7 @@ http {
{{ $cfg.ServerSnippet }}
{{ end }}

{{ template "CUSTOM_ERRORS" (buildCustomErrorDeps $all.ProxySetHeaders $cfg.CustomHTTPErrors) }}
{{ template "CUSTOM_ERRORS" (buildCustomErrorDeps $all.ProxySetHeaders $cfg.CustomHTTPErrors $all.EnableMetrics) }}
}
## end server {{ $server.Hostname }}

Expand Down Expand Up @@ -679,7 +683,7 @@ http {
proxy_pass http://upstream_balancer;
}

{{ template "CUSTOM_ERRORS" (buildCustomErrorDeps $all.ProxySetHeaders $cfg.CustomHTTPErrors) }}
{{ template "CUSTOM_ERRORS" (buildCustomErrorDeps $all.ProxySetHeaders $cfg.CustomHTTPErrors $all.EnableMetrics) }}
}
}

Expand Down Expand Up @@ -800,6 +804,7 @@ stream {
{{/* definition of templates to avoid repetitions */}}
{{ define "CUSTOM_ERRORS" }}
{{ $proxySetHeaders := .ProxySetHeaders }}
{{ $enableMetrics := .EnableMetrics }}
{{ range $errCode := .ErrorCodes }}
location @custom_{{ $errCode }} {
internal;
Expand All @@ -821,7 +826,9 @@ stream {

proxy_pass http://upstream_balancer;
log_by_lua_block {
Copy link

Choose a reason for hiding this comment

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

Cloudn't the whole log_by_lua_block be deactivated here?

Copy link
Member

Choose a reason for hiding this comment

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

We could. But it makes customizing Nginx config template harder. For example in my use case we call custom Lua module in log_by_lua phase. If the whole log_by_lua_blok is disabled we would have to change more lines whereas now we just append our custom logic after line 831.

{{ if $enableMetrics }}
monitor.call()
{{ end }}
}
}
{{ end }}
Expand Down Expand Up @@ -922,7 +929,7 @@ stream {
{{ $server.ServerSnippet }}
{{ end }}

{{ template "CUSTOM_ERRORS" (buildCustomErrorDeps $all.ProxySetHeaders (collectCustomErrorsPerServer $server)) }}
{{ template "CUSTOM_ERRORS" (buildCustomErrorDeps $all.ProxySetHeaders (collectCustomErrorsPerServer $server) $all.EnableMetrics) }}

{{ $enforceRegex := enforceRegexModifier $server.Locations }}
{{ range $location := $server.Locations }}
Expand Down Expand Up @@ -1087,7 +1094,9 @@ stream {
waf:exec()
{{ end }}
balancer.log()
{{ if $all.EnableMetrics }}
monitor.call()
{{ end }}
}

{{ if (and (not (empty $server.SSLCert.PemFileName)) $all.Cfg.HSTS) }}
Expand Down