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

collector collisions #128

Closed
acpana opened this issue Oct 11, 2021 · 1 comment · Fixed by #129
Closed

collector collisions #128

acpana opened this issue Oct 11, 2021 · 1 comment · Fixed by #129

Comments

@acpana
Copy link
Contributor

acpana commented Oct 11, 2021

Overview

IIUC these lines https://github.com/armon/go-metrics/blob/master/prometheus/prometheus.go#L129-L134 will create a collision for the Collector type if we attempt to register more than one PrometheusSink.

Repro

One repro scenario is described here hashicorp/consul#11273 .

Also, I have a test can repro this and will comment below

Proposed Fixes

  • IMO, the way we implement .Describe() needs to be reworked
@acpana
Copy link
Contributor Author

acpana commented Oct 11, 2021

Test to repro:

  • two different sinks
  • errs out with err = duplicate metrics collector registration attempted

func TestNewPrometheusSink(t *testing.T) {

	gaugeDef := GaugeDefinition{
		Name: []string{"my", "test", "gauge"},
		Help: "A gauge for testing? How helpful!",
	}
	// PrometheusSink config w/ definitions for each metric type
	cfg := PrometheusOpts{
		Expiration:         5 * time.Second,
		GaugeDefinitions:   append([]GaugeDefinition{}, gaugeDef),
		SummaryDefinitions: append([]SummaryDefinition{}),
		CounterDefinitions: append([]CounterDefinition{}),
	}

	sink1, err := NewPrometheusSinkFrom(cfg)

	reg := prometheus.DefaultRegisterer

	if err != nil {
		t.Fatalf("err = %v, want nil", err)
	}

	gaugeDef2 := GaugeDefinition{
		Name: []string{"my2", "test", "gauge"},
		Help: "A gauge for testing? How helpful!",
	}

	cfg2 := PrometheusOpts{
		Expiration:         15 * time.Second,
		GaugeDefinitions:   append([]GaugeDefinition{}, gaugeDef2),
		SummaryDefinitions: append([]SummaryDefinition{}),
		CounterDefinitions: append([]CounterDefinition{}),
	}

	sink2, err := NewPrometheusSinkFrom(cfg2) // errors out
        ...
}

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 a pull request may close this issue.

1 participant