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 for Alamofire Statistical Metrics? #76

Closed
gaplo917 opened this issue Mar 9, 2017 · 18 comments
Closed

Support for Alamofire Statistical Metrics? #76

gaplo917 opened this issue Mar 9, 2017 · 18 comments

Comments

@gaplo917
Copy link
Contributor

gaplo917 commented Mar 9, 2017

From Alamofire documentation, DataResponse provided a lot of useful information.

Statistical Metrics

Timeline

Alamofire collects timings throughout the lifecycle of a Request and creates a Timeline object exposed as a property on all response types.

Alamofire.request("https://httpbin.org/get").responseJSON { response in
    print(response.timeline)
}

The above reports the following Timeline info:

  • Latency: 0.428 seconds
  • Request Duration: 0.428 seconds
  • Serialization Duration: 0.001 seconds
  • Total Duration: 0.429 seconds

Does RxAlamofire currently provides such API?

@sergdort
Copy link
Member

sergdort commented Mar 10, 2017

Hi, @gaplo917
Currently we don't have a this option. I believe what you want is method that would return Observable<DataResponse> right?

@gaplo917
Copy link
Contributor Author

@sergdort exactly! My project needs to time the API later... It would be super nice if RxAlamofire can return Observale that include all useful meta data.

@simoami
Copy link

simoami commented Apr 17, 2017

@sergdort Any workaround for this? I have a different use case. The backend error message contains a key which is needed to identify what causes a validation error. http code is 400 but it's not enough. The error key provides additional details for a signup function, examples can be invalid_password, or email_already_exists.

@freak4pc
Copy link
Member

freak4pc commented May 4, 2017

I'm not 100% in-the-know of the structure of RxAlamofire but if you need a workaround, a simple wrapper function would assist here.

Something like this would work @gaplo917 @simoami
If I'll have some time I can look into how to add this into RxAlamofire after I learn the code there a bit :)

public func requestJSON(_ method: Alamofire.HTTPMethod,
                        _ url: URLConvertible) -> Observable<DataResponse<Any>> {
    return Observable.create { observer in
        let request = Alamofire.request(url)

        request.responseJSON { resp in
            observer.onNext(resp)
            observer.onCompleted()
        }

        return Disposables.create {
            request.cancel()
        }
    }
}

@freak4pc
Copy link
Member

freak4pc commented May 4, 2017

I added code to allow this directly from RxAlamofire but

  1. I'm not sure of the naming (requestJSONData, as to indicate it returns a DataResponse)
  2. I don't know Alamofire well enough to say if this is intuitive. Could anyone weigh in ? @kzaher @sergdort

If this makes sense I can submit a PR and we can keep the discussion going there.

4e0002d3-bc93-46a1-b218-a78a3859b016

@kzaher
Copy link
Member

kzaher commented May 4, 2017

@freak4pc maybe just RxAlamofire.dataResponse? ¯\_(ツ)_/¯

@freak4pc
Copy link
Member

freak4pc commented May 4, 2017

@kzaher It comes specifically from responseJSON in Alamofire e.g.

Alamofire.request(...).responseJSON { resp in 
 /// resp is DataResponse<Any>
}

and also the naming convention across seems to be that
requestJSON returns a result, so that might be a confusing naming convention ?

Might need to have other overloads for raw data etc but the basic use case here is JSON response

@kzaher
Copy link
Member

kzaher commented May 4, 2017

@freak4pc I think ****response****JSON**** might make sense since the original method is responseJSON. Somebody who is in the "zone" should figure out what it should exactly be :)

@freak4pc
Copy link
Member

freak4pc commented May 4, 2017

Yup thats why I tagged @sergdort who is definitely more active than me on this :) Using the library and maintaining, that is.

I'm also in favor of responseJSON but that gets "requestJSON" to not make much sense :)

The original responseJSON belongs to the response of a request() method (e.g. DataRequest).
Having similar syntax to Alamofire here would be nice but its not consistent with RxAlamofire right now so I'm not sure that's a good idea as well (e.g. RxAlamofire.request(...).responseJSON)

where request returns Observable<DataRequest> and responseJSON is something like

extension ObservableType where E == DataRequest {
    func responseJSON() -> Observable<DataResponse<Any>> {
        // some sort of flatmapping of self
    }
}

Some feedback from the community would be great here. Probably involving this level of complexity is less of a good idea. I think leaving everything under RxAlamofire.method is easier/more understood, so it's just a naming convention thing.

@freak4pc
Copy link
Member

freak4pc commented May 4, 2017

This is actually probably the nicest syntax for this, I reckon.
Thoughts? @kzaher @sergdort

5ea7ed60-768e-4f49-8957-ea8c1a189c79

I think it feels very nice and natural (also similar to how non-Rx Alamofire works), I'm comfortable with pushing a PR for it.

@simoami
Copy link

simoami commented May 4, 2017

@freak4pc @kzaher
not sure how relevant this is or if there's a simpler way but I worked around my use case, that is to fetch the error message and other attributes from the response:

request(method, path, parameters: parameters, encoding: encoding, headers: headers)
    .flatMap { (req: DataRequest) in
        return req.validate({ (request, response, data) -> Alamofire.Request.ValidationResult in
            // Only check for known error codes
            if self.isHTTPSuccess(code: response.statusCode) { return .success }
            let (name, message) = data != nil ? getHttpError(data!) : (nil, nil)
            // do something with error name and message
        })
        .rx.json()
    }

func getHttpError(_ json: Data) -> (String?, String?) {
    var errorName: String?
    var errorMessage: String?
    let d = try? JSONSerialization.jsonObject(with: json, options: JSONSerialization.ReadingOptions.allowFragments)
    if let data = d as? Dictionary<String, AnyObject> {
        if let message = data["error"]?["message"] {
            errorMessage = message!
        }
        if let name = data["error"]?["name"] {
            errorName = name
        }
    }
    return (errorName, errorMessage)
}

@freak4pc
Copy link
Member

freak4pc commented May 4, 2017

Thanks @simoami for sharing!
While that works well specifically for your case, the original question was about exposing the DataResponse object that currently has no good method of being accessed. I'm glad you were able to resolve your issue but it doesn't exactly relate to the original question. Also, with having the new syntax you could easily just access the DataResponse object which might reduce some of the headache you're having here (even though that is a bit more specific).

@simoami
Copy link

simoami commented May 4, 2017

@freak4pc I'm aware of the difference to the original question, which is why I keep referring to mine as a specific "use case" for fetching response data. btw, mind pointing me to the new syntax? I didn't check if a new update was published to make the response accessible. would love to incorporate!

@freak4pc
Copy link
Member

freak4pc commented May 4, 2017

Hey @simoami - Take a look here, I just pushed a PR explaining my thoughts on how to expose DataResponse. I'd love your feedback if it makes sense to you as an active user!

@simoami
Copy link

simoami commented May 4, 2017

Nice, great extension. It definitely looks easier. Will it also run onNext with other http status codes? say 401 or 500?

@freak4pc
Copy link
Member

freak4pc commented May 4, 2017

No. Errors don't come back as onNext event, they come back as onError events

@freak4pc
Copy link
Member

Fixed by #79

@simoami
Copy link

simoami commented May 25, 2017

Thanks @freak4pc.

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

No branches or pull requests

5 participants