Update reactive graph sample data to include time

[jcheng5]

cc @bborgesr

[jcheng5]

I'm going to merge this unless anyone has objections

[wch]

Go for it.

[jcheng5]

@bborgesr I think the time elapsed needs to be in separate DOM nodes from the existing label for a couple of reasons. 1) It'd be great if the elapsed time could be shown in a different typeface (proportional instead of fixed-width), smaller size, maybe bold, maybe lighter color. 2) The length of the label can be arbitrarily long and fades out as it gets longer. See this (contrived) example:

library(shiny)

ui <- fluidPage(
  plotOutput("plot")
)

server <- function(input, output, session) {
  reactive({
    df <- cars
    df <- head(df)
    df
  }) -> r

  output$plot <- renderPlot({
    plot(r())
  })
}

shinyApp(ui, server)

It's pretty unlikely you'd use -> instead of <- but the point is that any creation of an output or reactive that doesn't match the "common" pattern will end up having the entire code chunk put in as the label rather than just the name.

[bborgesr]

@jcheng5 Implemented those changes and also:

1. Made this new feature optional (i.e. by default showReactLog() displays the time elapsed but the user can also specify showReactLog(time=FALSE) to not have this new information cluttering up the graph...) What do you think? (although I'm not sure if I implemented it in the best way, since there doesn't really seem to exist an interface for graph.R to communicate with the JS file)

2. I thought it best to have the time elapsed info above the node's label for consistency -- in the cases where the label spans multiple lines, it would still be in the same place as in all other nodes

3. Removed the multilineTextNode() function because it wasn't actually getting used anywhere...

[jcheng5]

renderReactLog is also called from middleware-shiny.R, in this case it'll be missing the time argument because renderReactLog doesn't specify a default value.

[jcheng5]

Can we go quite a bit darker with the color? I find it a bit hard to read right now. How does #444444 look to you?

[jcheng5]

Usually you want to keep the data in its "native" form until you're ready to display it (i.e. store the timeElapsed float on the node, not the fully formatted string). If we decide we want to show the number in a different color or weight than the "time elapsed" label, then we can do that easily.

[jcheng5]

Other than the notes I've made, this looks great! It really makes me itch to want to pull forward all the improvements to the reactlog though :) Or at least to make this data easily exportable as a nice data frame, there's all sorts of interesting visualizations you could imagine on this data...

[bborgesr]

Yeah, I agree that making this data exportable (and maybe adding a few guidelines and examples of how it might be useful or how best to visualize it) would be great... (This might be especially useful for the larger apps that begin to test the limitation of our current visualization).

A few notes:

1. To make sure that we can call renderReactLog from middleware-shiny.R with the right default, I've changed it to renderReactLog <- function(sessionToken = NULL, time = TRUE). In an ideal world, this would mean that I would just have showReactLog <- function(...) (my first thought). The problem with this is that the resulting documentation for showReactLog is obscured (because the time argument is not shown)... So, now we've got the ugliness of both renderReactLog <- function(sessionToken = NULL, time = TRUE) and showReactLog <- function(time = TRUE)... I'm not sure what's the best solution to this...

2. You mean "go darker in the color" for the time elapsed info, right? (I didn't change the node label's color.) I had it green to be able to make it lighter (since it's not really a very important piece of information for most apps) while also making it very clear that it's different from the node label. I've changed it to your suggestion now, which is successful in distinguishing between the two labels, but I think it is maybe drawing too much attention to the time elapsed (because it's darker than everything else...). Although it is kind of nice to keep everything in shades of gray when it's not active...

3. Stored the timeElapsed float on the node instead of the fully formatted string (it does make a lot more sense now that I think about it...)

4. I've also decided to make sure that we're not showing any time elapsed info while the node is active/running (it could be confusing to the user...)

[jcheng5]

I think the travis failures are due to the new version of htmltools I just pushed to CRAN, which includes a new argument in the htmlTemplate function. I'll fix this in a separate PR.

[bborgesr]

Oh, good. I was scratching my head trying to figure out what I broke... :)

[bborgesr]

After some experimentation, I found that I like this color scale for the time labels. It's a 6-step monochromatic red scale ranging from a very pale red/beige to a dark wine red. I got it from colorbrewer, which is supported by d3 (lib/colorbrewer). Unfortunately, this script has to be loaded in addition to the regular d3 script. For now, I'm loading it directly from github (because I wanted to experiment with lots of scales and because I thought it might be nice for app authors to eventually have access to that and be able to pass it in as a parameter). However, we could also just decide to only include the particular color array we settle on. So let me know what you think about this particular color array... I feel like it's not too distracting (because it's monochromatic), but still achieves the goal of drawing the user's attention to the bottlenecks of the code...

To map the domain of time differences into the colors, I find the minimum and maximum time differences in the log data -- so that the time elapsed info is always normalized for that particular run of an app.

On a completely unrelated note, @jcheng5 -- I noticed that, in the beginning of the processMessage() function, you had a console.log(JSON.stringify(data));. I took it out for now, because we don't really want to have all that information dumped into the console, right? I had an eye out for that because of our talk yesterday about possible security issues. Let me know if I'm misunderstanding things.

[jcheng5]

It's fine to remove that console.log but there's not really security implications; if the data has made it to client-side JavaScript, it's already compromised, it doesn't matter if you print it to the console or not.

I wouldn't bother making the color scheme configurable at this time--let's just grab the colors we need and credit colorbrewer.

I'm not sure whether it makes sense to normalize the colors against the app's run times. If a reactive takes only 20ms but it happens to be the slowest reactive in an app, should it be dark red?

[bborgesr]

Yes, the slowest reactive in any app will always be dark red and the fastest will always be light red. I take your point that this might be strange at first, but I don't much like the alternative: if all your reactives are under 20ms (but there's a spread -- 1ms, 10ms, 20ms) if you don't normalize it, then the colors will be all the same(ish) and that defeats the purpose of color-coding the time labels.

On the other hand, as long as we explain that the colors are always normalized, I think we could make people look at this as a metric for their app only and identify what is taking longer in their app (even if it's super fast compared to other apps).

Also, it would be hard to define a fits-all domain because different apps can have really different run times...

[jcheng5]

OK. Let's start with that and see how it feels.

[bborgesr]

@jcheng5, @wch I think this is ready to merge