Repository navigation
Homeval analytics dashboard - #142
Conversation
|
@jeancochrane |
jeancochrane
left a comment
There was a problem hiding this comment.
Awesome start! I'm only about halfway through, and haven't started looking at the charts yet, but I wanted to pause and give feedback on the data transformation so that we can focus on that first. Let me know if any of my comments or questions are unclear and I'm happy to talk them through.
| # Cluster ID is a way to identify duplicate requests within the same query. | ||
| # We build it on one second intervals. | ||
| # When there are two pins, we separate them. | ||
| # For these, we remove the NA observations since we don't know which category | ||
| # they belong to. Otherwise we leave in NA pins for relevant metadata. |
There was a problem hiding this comment.
[Suggestion, optional] Nice work identifying that we need to deduplicate clusters of requests. However, I think there's an opportunity to simplify this clustering quite a bit so that we don't need to tune our notion of a cluster ID.
Stepping back a bit, the primary reason that we experience clusters of requests is that each HomeVal report requests a bunch of static files every time a user loads the page. These static files should all have a Referer header pointing to the HomeVal report that requested them, and since we currently parse PINs from the Referer field, we get a duplicate PIN for each static file request.
If we implement my comment above to stop parsing pins from the Referer field, I think the particular issue where static file requests are causing duplicate counts for PINs will go away on its own. Once we exclude static files from our PIN parsing, I don't think we'll need to do any further refinement to get counts of requests by PIN. However, we'll still need to keep in mind that a majority of the raw count of requests (i.e. not counts of individual PINs) will be for static files, which is rarely interesting for analysis. Instead of clustering PINs, I think we should instead tag all static file requests so that we can group them separately for analysis. This should be pretty easy to do by just checking for rows whose URI stem starts with the string /static/.
I'll add more detail here about what we should do with these tagged static file requests once I review the charts below, so I can figure out exactly what grouping makes sense.
There was a problem hiding this comment.
There's other values such as /apple-touch-icon-precomposed.png so I used starting with the four digit year, which seems like a more standardized metric (even though there may be able to be a misclicked 5 digit value, I'm not sure if that's possible).
Co-authored-by: Jean Cochrane <jeancochrane@users.noreply.github.com>
Co-authored-by: Jean Cochrane <jeancochrane@users.noreply.github.com>
Co-authored-by: Jean Cochrane <jeancochrane@users.noreply.github.com>
Co-authored-by: Jean Cochrane <jeancochrane@users.noreply.github.com>
Co-authored-by: Jean Cochrane <jeancochrane@users.noreply.github.com>
Co-authored-by: Jean Cochrane <jeancochrane@users.noreply.github.com>
Yup, so we want to create a new folder environment for this? |
No, I don't think so -- these R environment files should live in |
jeancochrane
left a comment
There was a problem hiding this comment.
I think this is just about ready to go! Some small thoughts below, but nothing serious.
One thing I totally forgot about when writing the issue: It might be useful to get a rough sense of the distributions of the time-taken and time-to-first-byte fields, which measure the amount of time it takes for the server to complete specific actions. Would you be down to take a quick stab at adding simple charts for those two metrics? I think it'll be fine to combine both query + static requests, and maybe create a new section called something like "Request time" with two tabs, "Time to First Byte" (time-to-first-byte) and "Time To Last Byte" (time-taken). Does that make sense to you?
| data_class <- data %>% | ||
| filter(uri_indicator == "Query") %>% | ||
| # Make sure we identify PINs which are incorrect or not joined to assessment_card | ||
| # For example PIN 14110010074923 |
There was a problem hiding this comment.
[Thought, non-blocking] I do think the nulls are useful in general, since they give a rough count of PINs/requests that might be getting dropped in the join! I think it would be useful to roll it out to any charts that don't have it where you think it would be feasible.
Co-authored-by: Jean Cochrane <jeancochrane@users.noreply.github.com>
Co-authored-by: Jean Cochrane <jeancochrane@users.noreply.github.com>
| tidyr::unnest_wider(parsed, names_repair = "minimal") %>% | ||
| mutate( | ||
| pin = coalesce( | ||
| str_extract(`cs-uri-stem`, "(?<=/)\\d{14}(?=\\.html)"), |
There was a problem hiding this comment.
There were a couple pins which just didn't have the .html suffix.
|
Updating the renv to get out of the digest lock has created a cyclical lintr / styler issue. Don't know if we want to work on that here or in another issue. Edit: I tried to manually revert the |
jeancochrane
left a comment
There was a problem hiding this comment.
Awesome work! I think this dashboard is just about ready to go. Thanks for your patience during a few rounds of review as we narrowed down the specification.
Updating the renv to get out of the digest lock has created a cyclical lintr / styler issue. Don't know if we want to work on that here or in another issue.
I think I was able to figure out the problem in #143, though I don't think we should merge that PR ibecause the diff is weird. Here are the steps I took to resolve the pre-commit errors, try taking these steps on your PR to see if they work:
- Update
.lintrto disablereturn_linter, which raises an error by default in new versions of lintr (5d032e5) - Tweak some lines in the 2-PIN comparison report to fix indentation problems (37a3d3f)
- Merge the
mainbranch into your branch to get rid of deprecated R files that we removed in #141 that are now causing lintr errors (505b28d)
| mutate( | ||
| reason_clean = case_when( | ||
| is.na(reason_report_ineligible) ~ "Un-joined PIN", | ||
| TRUE ~ as.character(reason_report_ineligible) | ||
| ) | ||
| ) %>% |
There was a problem hiding this comment.
[Suggestion, optional] As long as we're adjusting the definition of this column in the previous chart ("Type of Ineligibility Reason"), it might be worthwhile to factor it out in such a way that the three ineligibility charts can share it, perhaps by assigning the output to a new variable -- particularly since it seems like we may have missed this column in the third chart ("% Over Time). I'll leave it up to you to decide if you think that's worthwhile, though, or if you'd rather just change the 3 charts separately.
| # keep ineligible or null eligible cases. | ||
| # This allows us to find unjoined pins | ||
| is_report_eligible %in% c(FALSE, NA), | ||
| !is.na(pin) # Removes queries which do not have a PIN associated with it |
There was a problem hiding this comment.
[Question, non-blocking] Why remove these rows, given that we're including them in the "Type of Ineligibility Reason" chart above?
There was a problem hiding this comment.
When I was looking at the html suffix, there were just two pins which didn't comply with it, one with a pin10 and one with hyphens. With that being said, I'll take out the line, you are right that it shouldn't be in.
There was a problem hiding this comment.
If you are referring to this line
is_report_eligible %in% c(FALSE, NA),
it's because eligible pins have an NA value for the reason, so this separates the unjoined pins from the eligible pins as they are two distinct categories.
| ```{r} | ||
| data %>% | ||
| filter( | ||
| !is.na(reason_report_ineligible), |
There was a problem hiding this comment.
[Question, non-blocking] Why remove unmatched PINs from this chart, given that we show them in the previous two charts?
Co-authored-by: Jean Cochrane <jeancochrane@users.noreply.github.com>
Co-authored-by: Jean Cochrane <jeancochrane@users.noreply.github.com>
jeancochrane
left a comment
There was a problem hiding this comment.
Nice work! This is just about ready to go. The scraper issue is making me realize that we might benefit from some controls that allow us to control the filters on the queries. I opened up #144 to track that effort, but in the meantime, I think this PR is ready to merge.
Co-authored-by: Jean Cochrane <jeancochrane@users.noreply.github.com>
Co-authored-by: Jean Cochrane <jeancochrane@users.noreply.github.com>

This adds a pin-val analytics dashboard.
uaparserjspackage. Although there is a good deal of commenting on the internet which says the methods are not particularly reliable.