Network Requests #85
Network Requests #85
Conversation
src/mux-analytics.brs
Outdated
|
|
||
| props = {} | ||
| props.request_start = message.request_start | ||
| props.request_hostname = message.request_hostname |
There was a problem hiding this comment.
we probably, sadly, need to make sure none of these are invalid, or catch/handle invalid later down the pipe
There was a problem hiding this comment.
the ones I didn't do the invalid "checking" are the ones that say that are required in the docs, do we want to log an error and return if they are invalid or just check and keep going?
There was a problem hiding this comment.
even saying they're required in the docs, we need to make sure that the customer actually provides them. This is a customer-facing API, and we can't trust them to send in everything, so yeah I think you're right that we need to make sure not to throw errors or cause problems down the line. If a required field is not provided with a value, then I agree we should reject the call
Interested in @daytime-em and @CFKevinRef thoughts here as well
There was a problem hiding this comment.
-
I checked the other bandwidth metric implementations and it looks like we do allow request events to go up without most of this data. For parity's sake, I think we could do the same here
-
We should require
type(as in,requestcompleted,requestfailed,requestcanceled) since those wind up being theeproperty for the event, which is definitely required -
I don't see implementations of request throughput or latency. Those are currently calculated client-side, so we'd need to implement them here as well. (js implementation for reference).
- Even though we could try to implement latency/throughput on the processor, our planned implementation for sub-view metrics would still skip the processor (afaik) so I wouldn't want to rely on server-side calculations until we know for sure what we want to do there.
src/mux-analytics.brs
Outdated
| end sub | ||
|
|
||
| prototype.requestHandler = sub(message as Object) | ||
| requestVariant = message.request_variant |
There was a problem hiding this comment.
we don't use request_variant anywhere else, I don't think. this is meant to be the same idea as requestfailed vs requestcompleted vs requestcanceled right? Or was there some other prior art you were referencing with this?
There was a problem hiding this comment.
I think the idea of request_variant is that the customer provided us with the "type" (requestcompleted, requestfailed or requestcanceled), because otherwise we wouldn't know which type of request they want.
Maybe I understood that incorrectly, but that was my thinking of knowing which type of request it was. It would need to be documented obviously.
There was a problem hiding this comment.
That makes sense, yeah. I see how it's used later down the line now. I think this makes sense from an API perspective, but also would love @daytime-em / @CFKevinRef to chime in if they think there's a better API.
Given that we're just just having a single request field on the MuxTask that they set with a dictionary, we need a field like this, and the only thing I would think would be something like type instead of request_variant, but that's quite minor. Functionally, I think you have it working like we want
There was a problem hiding this comment.
Agreed. One 'request' field on the node with a 'type' sub-field seems like a reasonable choice for the API, considering how brightscript works
I like type as a name for the sub-field.
There was a problem hiding this comment.
changed it from request_variant to type (request_type is already in use)
No description provided.