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

Feature request: convert ws-auth to produce numerical stats. #4915

Closed
wants to merge 4 commits into from

Conversation

rektide
Copy link

@rektide rektide commented Jan 16, 2017

Short description

Currently stats are produced as strings, even though they are numbers. This makes them very hard to consume from metrics systems (we're using New Relic Insights at work).

Checklist

I have:

  • read the CONTRIBUTING.md document
  • compiled the code
  • [(could not figure out how to do this. regression-test instructions confusing, not direct!!)] ran tests on the code
  • included documentation (including possible behaviour changes)
  • documented the code
  • added regression tests
  • added unit tests

@zeha
Copy link
Collaborator

zeha commented Jan 16, 2017

Run tests for ws/api:

make ...
cd regression-tests.api 
./runtests authoritative

Ensure you have python + virtualenv installed first.

Atm ws-recursor has very important code commented out to make this
compile.
@zeha
Copy link
Collaborator

zeha commented Jan 16, 2017

  • Recursor also needs conversion
  • Some stats are 64bit items, so type for them can't be int

@zeha zeha added the rec label Jan 16, 2017
@rektide
Copy link
Author

rektide commented Jan 16, 2017

Still working through this. First go at recursor, but it's a little more intensive & I have less knowledge of the project right now which will slow down some of my manual testing some.

I'm not sure whether "unsigned long" or uint64_t is preferred? The latter is just a typedef, no? I see some of each in different points in the codebase.

@@ -63,7 +63,7 @@ private:
static bool s_init;
};

std::map<std::string, std::string> getAllStatsMap();
std::map<std::string, int> getAllStatsMap();
Copy link
Collaborator

Choose a reason for hiding this comment

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

mismatched type between decl here and impl below.

@zeha
Copy link
Collaborator

zeha commented Jan 16, 2017

I'm not sure whether "unsigned long" or uint64_t is preferred? The latter is just a typedef, no?

unsigned long is only guaranteed to fit 32bits.

Had momentarily just been 'int' in this conversion, had failed to update
this one usage.
@Habbie
Copy link
Member

Habbie commented Jan 18, 2017

Many JSON implementations limit integers to 53 bits; is this a concern?

@zeha
Copy link
Collaborator

zeha commented Jan 18, 2017

Right, I think we're limited to double actually?

@Habbie
Copy link
Member

Habbie commented Jan 18, 2017

@zeha exactly, which does lossless integers up to something around 53 bits (signed, I think)

@rektide
Copy link
Author

rektide commented Jan 18, 2017

JSON the spec has no limit. Unfortunately as per dropbox/json11#16 the JSON library we use insists that round-tripping a number through JSON in JS has higher precedence for their implementation than the spec, and refuses to offer any means to use large integers (tagged above issue wontfix).

So yes, metrics are- just before conversion to json- cast to double. Yes, numbers get imprecise after 53 bits, not counting the sign bit, as Habbie says.

@Habbie Habbie changed the title Feature requst: convert ws-auth to produce numerical stats. Feature request: convert ws-auth to produce numerical stats. Mar 20, 2017
@pieterlexis
Copy link
Contributor

@aerique I believe you were working on similar things with dnsdist, can you chime in?

@aerique
Copy link
Member

aerique commented Jun 2, 2017

Yes, we 'fixed' it for dnsdist by casting to double as well. 53 bits should be enough for everyone.

See: #5234

@zeha
Copy link
Collaborator

zeha commented Jan 5, 2018

@rektide mind rebasing this?

@Habbie
Copy link
Member

Habbie commented May 31, 2018

ping?

@Habbie Habbie added this to the auth-helpneeded milestone Jun 7, 2019
@pieterlexis
Copy link
Contributor

Closing for lack of response. We'll do a Prometheus endpoint in 4.3 or 4.4

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants