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

Support :all as an aggregation mode in DirectFileStore #127

Merged
merged 3 commits into from
Jun 13, 2019

Conversation

Sinjo
Copy link
Member

@Sinjo Sinjo commented May 31, 2019

This is the beginning of the work for #107. I'm going to detail the plan I came up with in there.

Not mega happy with this implementation, so I've marked this PR as a draft for now. Will see if I can come up with anything better.

@Sinjo Sinjo force-pushed the sinjo-unaggregated-gauges branch from cad0a4f to 4f64dfc Compare June 11, 2019 17:53
@Sinjo
Copy link
Member Author

Sinjo commented Jun 11, 2019

Before I take this out of draft, I'm going to make pid into a globally reserved label as I mentioned in #107.

I think it's cleaner to disallow it completely rather than saying "If you're using the gem in this particular mode then it's okay otherwise we'll overwrite it, sucks to be you!"

@dmagliola
Copy link
Collaborator

Definitely the right call to make it reserved.

We want to support exporting each process's individual value for gauges.
To enable this, DirectFileStore needs a new aggregation mode - :all.

Signed-off-by: Chris Sinjakli <chris@gocardless.com>
@Sinjo Sinjo force-pushed the sinjo-unaggregated-gauges branch from f55b61f to 922a96c Compare June 12, 2019 14:03
Signed-off-by: Chris Sinjakli <chris@gocardless.com>
@Sinjo Sinjo force-pushed the sinjo-unaggregated-gauges branch from 922a96c to b0a49cd Compare June 12, 2019 14:03
Signed-off-by: Chris Sinjakli <chris@gocardless.com>
@Sinjo Sinjo force-pushed the sinjo-unaggregated-gauges branch from faa7e89 to e66ab1b Compare June 12, 2019 14:22
@Sinjo Sinjo marked this pull request as ready for review June 12, 2019 14:27
@Sinjo
Copy link
Member Author

Sinjo commented Jun 12, 2019

I've marked this as ready for review. As far as I can tell, the build failures are a (hopefully transient) RVM issue in the JRuby build.

$ rvm get stable
Downloading https://get.rvm.io
Downloading https://raw.githubusercontent.com/rvm/rvm/master/binscripts/rvm-installer.asc
Verifying /home/travis/.rvm/archives/rvm-installer.asc
gpg: Signature made Tue 23 Apr 2019 04:50:26 PM UTC using RSA key ID 39499BDB
gpg: Good signature from "Piotr Kuczynski <piotr.kuczynski@gmail.com>"
gpg: WARNING: This key is not certified with a trusted signature!
gpg:          There is no indication that the signature belongs to the owner.
Primary key fingerprint: 7D2B AF1C F37B 13E2 069D  6956 105B D0E7 3949 9BDB
GPG verified '/home/travis/.rvm/archives/rvm-installer'
Downloading https://github.com/rvm/rvm/archive/.tar.gz
curl: (22) The requested URL returned error: 404 Not Found
Could not download 'https://github.com/rvm/rvm/archive/.tar.gz'.
  curl returned status '22'.
Downloading https://bitbucket.org/mpapis/rvm/get/.tar.gz
curl: (22) The requested URL returned error: 404 Not Found
Could not download 'https://bitbucket.org/mpapis/rvm/get/.tar.gz'.
  curl returned status '22'.
Could not update RVM, please report to https://github.com/rvm/rvm/issues
The command "rvm get stable" failed and exited with 22 during .
Your build has been stopped.

Will take a look later if it turns out not to be transient.

@coveralls
Copy link

coveralls commented Jun 12, 2019

Coverage Status

Coverage remained the same at 100.0% when pulling e66ab1b on sinjo-unaggregated-gauges into 6ea675e on master.

@Sinjo
Copy link
Member Author

Sinjo commented Jun 12, 2019

Yep, it was transient!

@Sinjo Sinjo requested a review from dmagliola June 12, 2019 15:42
Copy link
Collaborator

@dmagliola dmagliola left a comment

Choose a reason for hiding this comment

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

:shipit:

@Sinjo Sinjo merged commit 3652cf7 into master Jun 13, 2019
@Sinjo Sinjo deleted the sinjo-unaggregated-gauges branch June 13, 2019 12:34
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 this pull request may close these issues.

None yet

3 participants