10000 prometheus metrics for fleet by miekg · Pull Request #1415 · coreos/fleet · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content
This repository was archived by the owner on Jan 30, 2020. It is now read-only.

prometheus metrics for fleet #1415

Closed
wants to merge 6 commits into from
Closed

prometheus metrics for fleet #1415

wants to merge 6 commits into from

Conversation

miekg
Copy link
Contributor
@miekg miekg commented Jan 28, 2016

This PR add prometheus metrics to fleet.

@miekg
Copy link
Contributor Author
miekg commented Jan 28, 2016

Note: didn't add prometheus (and friends) in gopath.

@jonboulle jonboulle added this to the v0.13.0 milestone Jan 31, 2016
@jonboulle jonboulle mentioned this pull request Feb 6, 2016
Closed
@kayrus kayrus mentioned this pull request Mar 2, 2016
Open
@antrik
Copy link
Contributor
antrik commented Mar 15, 2016

@miekg before I try to review this in detail: on a quick glance, it looks like all the later commits just improve upon the original one, rather than making some distinct changes? In that case, they should probably all be squashed into a single commit.

You mentioned that you haven't added the prometheus dependency, but you didn't explain why?

One detail I noticed is that the copyright years are wrong. It should clearly be 2016 for the newly added file; and as far as I can see, the convention around here is to also bump the year of existing files when making substantial changes.

(Strictly speaking, when making changes, the original date should be kept along with the updated one; and also, since I guess you haven't signed over copyright to CoreOS Inc., they should mention your or your employer's name... But as far as I can see, these rules haven't been followed for fleet in the past either -- so not sure how to handle this...)

BTW, would it be feasible to add any unit tests and/or functional tests for this new feature?...

@jonboulle
Copy link
Contributor

@miekg I know your situation changed recently - let us know if you have
time to shepherd this through, otherwise one of us can take it over.

On Tue, Mar 15, 2016 at 6:07 PM Olaf Buddenhagen notifications@github.com
wrote:

@miekg https://github.com/miekg before I try to review this in detail:
on a quick glance, it looks like all the later commits just improve upon
the original one, rather than making some distinct changes? In that case,
they should probably all be squashed into a single commit.

You mentioned that you haven't added the prometheus dependency, but you
didn't explain why?

One detail I noticed is that the copyright years are wrong. It should
clearly be 2016 for the newly added file; and as far as I can see, the
convention around here is to also bump the year of existing files when
making substantial changes.

(Strictly speaking, when making changes, the original date should be kept
along with the updated one; and also, since I guess you haven't signed over
copyright to CoreOS Inc., they should mention your or your employer's
name... But as far as I can see, these rules haven't been followed for
fleet in the past either -- so not sure how to handle this...)

BTW, would it be feasible to add any unit tests and/or functional tests
for this new feature?...

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
#1415 (comment)

@miekg
Copy link
Contributor Author
miekg commented Mar 17, 2016

[ Quoting notifications@github.com in "Re: [fleet] prometheus metrics for ..." ]

@miekg I know your situation changed recently - let us know if you have
time to shepherd this through, otherwise one of us can take it over.

Thanks. I don't think I can spend cycles on this one...

Thanks for stepping in.

/Miek

Miek Gieben

@mwitkow mwitkow mentioned this pull request Mar 17, 2016
Closed
@kayrus kayrus self-assigned this Mar 31, 2016
@kayrus kayrus mentioned this pull request Mar 31, 2016
Merged
@joshix
Copy link
Contributor
joshix commented Apr 1, 2016

Superseded closed by #1524

@joshix joshix closed this Apr 1, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees

@kayrus kayrus

Labels
help wanted needs review
Projects
None yet
Milestone
v0.13.0
Development

Successfully merging this pull request may close these issues.
5 participants
@miekg @antrik @jonboulle @joshix @kayrus
0