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

Add MustCollector API for pusher #1067

Closed
wants to merge 1 commit into from
Closed

Add MustCollector API for pusher #1067

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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
10 changes: 10 additions & 0 deletions prometheus/push/push.go
View file
Open in desktop
Expand Up @@ -168,6 +168,16 @@ func (p *Pusher) Collector(c prometheus.Collector) *Pusher {
return p
}

// MustCollector works like Collector but registers any number of
// Collectors and panics upon the first registration that causes an
// error.
//
// For convenience, this method returns a pointer to the Pusher itself.
func (p *Pusher) MustCollector(c ...prometheus.Collector) *Pusher {
p.registerer.MustRegister(c...)
Copy link
Member

Choose a reason for hiding this comment

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

I don't think adding an API that panics to the pusher is the right solution here.
If I'm not mistaken, I understand from you comment that you want to have an API to add multiple collectors. Would a simple for loop not satisfy your needs?

p := push.New("http://example.org/metrics", "my_job").
for _, c := range []prometheus.Collector{myCollector1, myCollector2} {
    p.Collector(myCollector1)
} 
p.Grouping("zone", "xy").Client(&myHTTPClient).BasicAuth("top", "secret").Add()

If somehow you don't want to use this, I might consider making Collector method variadic.

func (p *Pusher) Collector(collectors ...prometheus.Collector) *Pusher {
      if p.error != nil {
            return p
      }
       for _, c := range collectors {
            p.error = p.registerer.Register(c)		
            if p.error != nil {
                  return p
            }
	}
	return p
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is myCollector1 registered successfully? We want to know the result when the collectors is being registered. Maybe that is why Registerer has the MustRegister API.

Copy link
Member

Choose a reason for hiding this comment

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

I see what you mean. We can consider adding a method the obtain the error from outside. I just don't want to add a method that panics in the pusher. None of the other APIs in the package do it. I'd prefer to keep the consistent behaviour.

return p
}

// Grouping adds a label pair to the grouping key of the Pusher, replacing any
// previously added label pair with the same label name. Note that setting any
// labels in the grouping key that are already contained in the metrics to push
Expand Down
16 changes: 16 additions & 0 deletions prometheus/push/push_test.go
View file
Open in desktop
Expand Up @@ -107,6 +107,22 @@ func TestPush(t *testing.T) {
t.Error("unexpected path:", lastPath)
}

// use MustCollectors, all good.
if err := New(pgwOK.URL, "testjob").
MustCollector(metric1, metric2).
Push(); err != nil {
t.Fatal(err)
}
if lastMethod != http.MethodPut {
t.Errorf("got method %q for Push, want %q", lastMethod, http.MethodPut)
}
if !bytes.Equal(lastBody, wantBody) {
t.Errorf("got body %v, want %v", lastBody, wantBody)
}
if lastPath != "/metrics/job/testjob" {
t.Error("unexpected path:", lastPath)
}

// Add some Collectors, with nil grouping, all good.
if err := New(pgwOK.URL, "testjob").
Collector(metric1).
Expand Down