Develop

[mm-gmbd]

No description provided.

[mm-gmbd]

There are a few nuances that will need to be addressed --

1. notice that the beforeOnReply handler gets called with two arguments, the original message and the full response payload. The distinction here is that the onReply handlers get called with the message and the data portion of the payload. The reason for sending the full response payload to the beforeOnReply handler is because it is being used as middleware, and as such I didn't want to muddy the contents of the data portion of the payload with what I was doing with the contents of the full message, so the handler needed access to the full payload... does this make sense?

2. Notice that the beforeOnData handler gets called with name and payload -- this is unlike any other handler and technically, name is in the payload so that's kind of redundant. I'm not sure how to make this fit with the current structure, so maybe it just gets called with payload and have that be it.

[mm-gmbd]

@ppetrosh - let me know what you think.

[mm-gmbd]

Also - this includes the change in _onAckReceived that only deletes the message from the sentQueue if there is no reply handler present (to allow for asynchronous reply). I know this isn't the final way this will be handled, but I went ahead and included it...

[ppetrosh]

Got it, thank you @mm-gmbd! Will review and take it over from here.

[ppetrosh]

@mm-gmbd regarding the beforeOnData and beforeOnReply we are not exposing any internal structures of the message to users. Why do you need that? Why don't you use the original message payload and supply it with all the required information?

[mm-gmbd]

The reason for using the full payload is because that allows additional things to be added to the message (e.g. timestamps as I mentioned above) in the middleware without impacting the data portion of the payload. If I have to take care to add profiling code to every reply/response, that would require a lot of extra work on the developer, whereas exposing the full payload (and letting the user get the data portion themselves) allows for much more flexibility.

[ppetrosh]

Hi @mm-gmbd, I'm not sure I understand ho onBeforeReply with a "full" payload helps there (the full means:

{
    "id" : id,
    "data" : data
}

Say you have some data which is your reply and some metadata that you want to use. Then you send the payload:

{
    "id" : id,
    "data" : {
        "metadata" : metadata,
        "data" : data
    }
}

and access them on the other side as payload.data or payload.metadata, the same way as you suggested. So we get the same result with a less API and complexity. What am I missing?

[imp-jenkins]

Can one of the admins verify this patch?

[mm-gmbd]

Hey @ppetrosh. Just saw the request for someone to verify this patch and I realized that I never responded to your question last year -- sorry!!

I had to re-read the thread in order to remember where everything stood, but to answer your latest question...

I think it may be handy to talk through this with a full example. Say I have a message that I want to send from Device->Agent, and then reply to that message from Agent->Device (via the built in MessageManager reply):

//**************
//Agent source
//**************
mm.on("add", function(data, reply){
  local a = data.a;
  local b = data.b;

  imp.wakeup(1.0, function(){
    reply (a+b);
  })
})


//**************
//Device source
//**************
server.log("Sending message to Agent to add!")
mm.send("add", {"a": 1, "b": 9})
.onReply(function(message, response){
  server.log("Result: "+response);
})

Now, let's say that, for whatever reason, there is a long delay in sending the message versus getting the response (much longer than the one second delay in the function). As a developer, I want to add some profiling to understand if it's the processing time on the Agent that is accounting for the delay, or if it is network latencies. I can think of two ways to accomplish this:

Method A: Add the Profiling to the Implementation Itself

This is not too big of a problem if there is one and only one message to profile, but in the event that there are multiple messages with different names (which I would assume is most use cases), then the developer has to touch every single line of implementation to add in the profiling, and then has to remove it all to take it back out.

//**************
//Agent source
//**************
mm.on("add", function(data, reply){
  local t1 = time(); //Keep track of when message is received
  local a = data.a;
  local b = data.b;

  imp.wakeup(1.0, function(){
    local t2 = time(); //Keep track of when Agent processing is complete
    local res = { //Modify response structure to include the original data plus the new metadata
      "data": a+b,
      "metadata": {
        "processingTime": t2-t1
      }
    }

    reply (res);
  })
})


//**************
//Device source
//**************
server.log("Sending message to Agent to add!")

local t1 = time(); //Keep track of when message was sent
mm.send("add", {"a": 1, "b": 9})
.onReply(function(message, response){

  local t2 = time(); //Keep track of when response was recevied
  local rtt = t2-t1;
  server.log("Result: "+response.data); //Modify log to account for new response data structure
  server.log("Agent Processing Time: "+response.metadata.processingTime);
  server.log("RTT: "+rtt);
  server.log("Network latency: "+rtt-response.metadata.processingTime);
})

Method B: Use Middleware to Implement Profiling

I think with a few optional middlewares, this could be cleaned up immensely and will profile all messages (rather than just the ones that the implementation is updated for).

However, I think there are some fundamental differences between my proposed approach (i.e. including the full payload) vs. only exposing the data portion to the middlewares. So... let's walk through them :)

Method B1: Only Expose the data Portion to Middleware

Now, let's consider your proposal of only providing the data portion of the full payload
Without exposing the internal structures and only providing data, the implementation now looks like:

//**************
//Agent source
//**************
mm.beforeOnData(function(name, data){
  //NOTE: Assume that, whatever is returned is the new `payload.data`
  return {
    "data": data, //Move the data into a "data" field in new table
    "metadata": { //Add metadata to a "metadata" field in new table
      "timeAgentReceived": time()
    }
  }
})

mm.beforeReply(function(message, data){
  //NOTE: Assume that, whatever is returned is the new `payload.data`
  return {
    "data": data.data,
    "metadata": {
      "processingTime": time() - data.metadata.timeAgentReceived
    }
  }
})

mm.on("add", function(data, reply){
  local a = data.data.a; //Now that the original data has moved into the sub "data" key in the table, I have to change my implementation code!
  local b = data.data.b; //Now that the original data has moved into the sub "data" key in the table, I have to change my implementation code!

  imp.wakeup(1.0, function(){
    local res = {
      "data": a+b,
      "metadata": data.metadata
    }

    reply (res);
  })
})


//**************
//Device source
//**************
server.log("Sending message to Agent to add!")

mm.beforeSend(function(message, enqueue, drop){
  message.metadata = {
    "timeSent": time();
  }
})

mm.beforeOnReply(function(message, data){
  local rtt = time() - message.metadata.timeSent;
  server.log("Agent processing time": data.metadata.processingTime);
  server.log("rtt: "+rtt)
  server.log("Network latency: "+(rtt-data.metadata.processingTime);
})

mm.send("add", {"a": 1, "b": 9})
.onReply(function(message, response){
  server.log("Result: "+response.data); //Now that the original data has moved into the sub "data" key in the table, I have to change my implementation code!
})

This is better than not having middleware at all (i.e. Method A), but you can see that the implementation code, on both the Device and the Agent, must be touched! Meaning that, for all messages, implementation code has to be updated to include and remove profiling such as this.

Method B2: Expose full Data Structure to Middlewares

This method allows for the highest level of flexibility and usefulness to the developer, as it allows them to fully implement middlewares by not affecting the data portion of the payload, while still having access to the data portion of the payload if that is useful.

//**************
//Agent source
//**************
mm.beforeOnData(function(name, payload){
  payload.metadata <- {
    "timeAgentReceived": time();
  }
})

mm.beforeReply(function(message, payload){
  payload.metadata = {
    "processingTime": time() - payload.metadata.timeAgentReceived;
  }
})

mm.on("add", function(data, reply){
  local a = data.a; //No code change to implementation!
  local b = data.b; //No code change to implementation!

  imp.wakeup(1.0, function(){
    reply (a+b);
  })
})


//**************
//Device source
//**************
server.log("Sending message to Agent to add!")

mm.beforeSend(function(message, enqueue, drop){
  message.metadata = {
    "timeSent": time();
  }
})

mm.beforeOnReply(function(message, data){
  local rtt = time() - message.metadata.timeSent;
  server.log("Agent processing time": data.metadata.processingTime);
  server.log("rtt: "+rtt)
  server.log("Network latency: "+(rtt-data.metadata.processingTime);
})

mm.send("add", {"a": 1, "b": 9})
.onReply(function(message, response){
  server.log("Result: "+response); //No code change to implementation!
})

Note that, providing the full payload (and therefore the ability to modify other bits of the message other than data), no implementation code has to be touched and the middleware implementation on both Device and Agent is very clean!

Let me know if this all helps...

[mm-gmbd]

Another general note -- in beforeSend you are already exposing the full DataMessage to the middleware, so for consistency it would make sense (with the other already-supported middlewares) to expose the full payload.

[ppetrosh]

thank you @mm-gmbd for providing the detailed example! a couple of comments:

1. In the current implementation ALL the middleware callbacks are consistent in the way they have the full message object is exposed. Please let me know which specific API call doesn't do so. Please NOTE: the on doesn't count as we are not sending the full message object over the wire.

2. I understand the use case, and it makes sense to me in general. And I agree that adding more APIs to the library might make this specific use case easier to implement. However a) the cost of it is an overloaded library interface which would make it hard to understand/apply for any other use case. I would be more comfortable to move forward with this proposal if we can validate it with another customer, who requests for it. But you are the only one so far... And b) that would also expand the scope of work for us to develop tests and verify all the corner use cases that the change introduces. Which might be hard to justify at this point...

3. For now I would recommend to probably extend MessageManager and add the functionality that you guys need. I'm not talking about forking the repository, but using the standard implementation just extend it the class and overwrite the methods that you need. If/when we see more demand for it, we'll incorporate your changes into the class. Does it make sense?

[mm-gmbd]

That all makes sense.

1. I didn't mean that any of the current API calls don't expose the full message object, I thought that your proposal was saying that we should only expose the data portion of the payload, which seems opposite from the current implementations.

2. Understood.

3. Yes, that makes sense. As the request was mostly centered around profiling messages when we were having Agent problems, I see this mostly as a debugging tool for now, so I'm fine with it being an extension as we are currently removed the use of these middlewares for now (but are very helpful when issues arise).

Can you make the extension and host it in the repository? Maybe with the stipulation that all edge cases haven't been tested and is mainly for debugging purposes at this point?

[ppetrosh]

Yes it probably makes sense to put it inside examples folder under this repository. That's going to show how you can extend the library as needed.

[betzrhodes]

Merging this with mm branch to keep reference to code. At this time there is no plan to merge this feature into a release.