Skip to content
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

Merged
merged 9 commits into from
Nov 5, 2024

Conversation

ldecicco-USGS
Copy link
Collaborator

As we look to improve our understanding of analytics, we want to have good flexibility to add custom signatures to the user agent.

Copy link

@jzemmels jzemmels left a 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") != "") {
Copy link

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?

Copy link

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 = ""))
}

Copy link
Collaborator Author

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.

@ldecicco-USGS ldecicco-USGS merged commit 8277747 into DOI-USGS:main Nov 5, 2024
7 of 8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants