-
Notifications
You must be signed in to change notification settings - Fork 87
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
Change custom UA to environmental variable #737
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! I appreciate the inclusion of the GitHub and GitLab CI pipeline tokens. I don't think my comments are necessary to address in this PR, so I'll approve the changes.
@@ -137,8 +137,8 @@ default_ua <- function() { | |||
|
|||
ua <- paste0(names(versions), "/", versions, collapse = " ") | |||
|
|||
if ("UA.dataRetrieval" %in% names(options)) { | |||
ua <- paste0(ua, "/", options()[["UA.dataRetrieval"]]) | |||
if (Sys.getenv("CUSTOM_DR_UA") != "") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a nice alternative to the options argument. Where do you think would be a good place to document the feature? Would it be something to advertise to power users, included in the getWebServiceData function documentation, or elsewhere since it applies to other functions too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another idea that we could potentially put off for later: what if we set the environment variable on package load to some random alphanumeric string if it isn't already set elsewhere? Then we'd be able to at least track requests originating from the same R session. Perhaps something like the following:
if (Sys.getenv("CUSTOM_DR_UA") == "") {
Sys.setenv("CUSTOM_DR_UA" = paste0(sample(c(LETTERS, 0:9), size = 12, replace = TRUE), collapse = ""))
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My gut says we start with advertising the change in NEWS and documentation. Then we can point a few power users to that and ask them to add it to their pipelines. THEN after you've had a chance to see how that helps (or not!) in your analytic work, we can think about adding this sort of session info. I'm worried it would just overwhelm the server log work.
As we look to improve our understanding of analytics, we want to have good flexibility to add custom signatures to the user agent.