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 json format for metrics #70

Merged
merged 6 commits into from
Nov 16, 2020
Merged

Conversation

amrutha-shanbhag
Copy link
Collaborator

Metrics format accepted as a query parameter "accept". Default format is prometheus. Other accepted format is "application/json".

@zegelin
Copy link

zegelin commented Nov 12, 2020

Let's change the query param to x-accept to match cassandra-exporter's behaviour.

As per the discussion in #shotover we should also support the HTTP Accept header.

@zegelin
Copy link

zegelin commented Nov 13, 2020

I also wonder if it'd be better to use Warp or another high-level HTTP server implementation.

From a very cursory read it looks like Warp makes it easier to handle headers and various things.

Getting Accept right is actually kinda complicated when using a low-level HTTP server implementation. See cassandra-exporter's HttpHandler for an example of the pain required.

@zegelin zegelin requested review from zegelin and benbromhead and removed request for benbromhead and zegelin November 13, 2020 00:27
@zegelin
Copy link

zegelin commented Nov 13, 2020

I also wonder if it'd be better to use Warp or another high-level HTTP server implementation.

BTW, Warp was literally the first thing I found from a DDG search. I'm a total Rust noobie — I have no idea if there's a better candidate HTTP library.

@amrutha-shanbhag
Copy link
Collaborator Author

I also wonder if it'd be better to use Warp or another high-level HTTP server implementation.

From a very cursory read it looks like Warp makes it easier to handle headers and various things.

Getting Accept right is actually kinda complicated when using a low-level HTTP server implementation. See cassandra-exporter's HttpHandler for an example of the pain required.

yeah, am struggling a bit with the headers. @benbromhead any thoughts on wrap ?

@benbromhead
Copy link
Member

So I haven't had a chance to look at the patch yet, but below are some general thoughts on a HTTP library / implementation

(tl;dr, it's complex and we want to do http in other parts of shotover, so lets just do quick and dirty right now even if its just a query param... that way we can do http right for the whole thing when we get to it)...

  • In order to support Databases like DynamoDB, Elasticsearch or Mongo we will need to have something that speaks HTTP (we'll need a HTTP source, like with have a CQL source and a RESP2 source).
  • I'm not sure of what the right HTTP library to use is, I'm hesitant to introduce something that has tower.rs as a dependency as it makes your write futures the old school way (e.g. implement poll etc) rather than async/await
  • The current implementation of metrics / log filtering is already a hack bolted onto the side. The right way to do it is actually have a HTTP source hooked into a prometheus / log filter destination (so it behaves and is configurable like any other transform chain).

So right now given that it's a pita to handle headers with what we have, let's choose x-accept as the q param so its compatible with other things in our ecosystem / what we want to do in the future and be done with it. Once we do prometheus/HTTP etc the right way it should then be easy to add the right header support.

@benbromhead
Copy link
Member

In fact iirc we already do use hyper (https://github.com/hyperium/hyper), which uses tower.rs under the hood, but at least it doesn't leak beyond the current implementation...

let output = observer.drain();
let output = match req.uri().query() {
(Some("x-accept=application/json")) => {
let builder = Arc::new(JsonBuilder::new().set_pretty_json(true));
Copy link
Member

Choose a reason for hiding this comment

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

given this is machine readable, we probably don't need to do pretty json

controller.observe(&mut observer);
let output = observer.drain();
let output = match req.uri().query() {
(Some("x-accept=application/json")) => {
Copy link
Member

Choose a reason for hiding this comment

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

It's kinda annoying the hyper won't even parse the query param for us and we need to match the whole string... but per my previous comment... oh well

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yeah, I tried few things, but looks like will need a better library to do stuff like that

@benbromhead
Copy link
Member

Ah yeah the broken build was my fault, rerunning should work fine now.

@benbromhead benbromhead merged commit 4c08c1b into main Nov 16, 2020
@benbromhead benbromhead deleted the support_json_format_for_metrics branch December 20, 2020 21:46
@conorbros
Copy link
Member

  • The current implementation of metrics / log filtering is already a hack bolted onto the side. The right way to do it is actually have a HTTP source hooked into a prometheus / log filter destination (so it behaves and is configurable like any other transform chain).

@benbromhead Can you elaborate on this?

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

4 participants