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

MANTA-4387 [mantav1] update Moray to use artedi v2 fixed buckets histograms #16

Merged
merged 2 commits into from
Jan 11, 2020
Merged

MANTA-4387 [mantav1] update Moray to use artedi v2 fixed buckets histograms #16

merged 2 commits into from
Jan 11, 2020

Conversation

khalfella
Copy link
Contributor

This small change updates moray to use artedi v2, mainly to make use of fixed buckets histograms introduced in v2. See testing notes in the ticket.

For some reason, I had to update the copyright in Makefile to 2020 in order for make check to pass.

@trentm
Copy link
Contributor

trentm commented Jan 11, 2020

For some reason, I had to update the copyright in Makefile to 2020 in order for make check to pass.

Yah, that is because of https://jira.joyent.us/browse/TOOLS-2400 unfortunately.

lib/server.js Outdated
@@ -153,6 +153,7 @@ function MorayServer(options) {
help: 'The node-fast CRC compatibilty mode of the Fast server'
}).set(crc_mode);

collector.FIXED_BUCKETS = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

@joshwilsdon Is this right? I can't tell from https://github.com/joyent/node-artedi/blob/master/docs/API.md#collectorfixed_buckets nor quickly from a scan of TritonDataCenter/node-artedi#17 how FIXED_BUCKETS is meant to be used.

If it is "always true in node-artedi v2", then is there a point in setting it here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

node-fast checks for FIXED_BUCKETS here.

Copy link
Contributor

Choose a reason for hiding this comment

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

@khalfella Per Josh's comment at #17 (review) I think you are not meant to set this. It is a value that is set by node-artedi automatically.

Copy link
Contributor

Choose a reason for hiding this comment

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

I commented on this same issue in #17 but the short answer is: no. It should not be doing this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@joshwilsdon - Thanks for making this clearer. I removed the line that sets FIXED_BUCKETS and confirmed that we still get correct histograms.

@khalfella khalfella merged commit 3942f2a into TritonDataCenter:mantav1 Jan 11, 2020
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.

3 participants