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

ENH: Add operating system to user-agent #344

Merged
merged 2 commits into from
Sep 8, 2022

Conversation

at88mph
Copy link
Contributor

@at88mph at88mph commented Jun 29, 2022

Add system to user-agent.

@codecov
Copy link

codecov bot commented Jun 30, 2022

Codecov Report

Merging #344 (5de6759) into main (197cd38) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main     #344   +/-   ##
=======================================
  Coverage   78.28%   78.28%           
=======================================
  Files          46       46           
  Lines        5493     5494    +1     
=======================================
+ Hits         4300     4301    +1     
  Misses       1193     1193           
Impacted Files Coverage Δ
pyvo/utils/http.py 100.00% <100.00%> (ø)

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

@msdemlei
Copy link
Contributor

I can't say I'm terribly happy to see pyVO leaking more information on the client than absolutely necessary, and I'm not convinced information on the platform people use count as "absolutely necessary". Then again, very few people care to tame their browsers' chattiness in that respect, so it's not like we're betraying secrets folks generally care about.

Still: What exactly is this supposed to help with/fix?

@at88mph
Copy link
Contributor Author

at88mph commented Jun 30, 2022

The use case is from the astroquery.alma client that wishes to have statistics, service optimization, and to track errors that are inconsistent across systems. I moved it up to pyvo as it's not unusual to contain the OS in the user-agent header and is perhaps better suited there.

@andamian
Copy link
Contributor

The question here is really if the OS information is something useful to have in general for service providers to improve their services. The astroquery.alma case, and other similar cases where user-agent or other headers need to be present or have certain values, can be easily solved by creating a custom session with the appropriate values that is then passed to service classes.

@andamian
Copy link
Contributor

andamian commented Jul 7, 2022

What do you think @tomdonaldson?

@bsipocz
Copy link
Member

bsipocz commented Jul 9, 2022

Having some stats on the platform is also rather useful info to know how much effort to put into fixing exotic bugs. E.g. we regularly receive reports from packagers about issues with some edge cases, knowing whether there is any realistic chance that those affect actual users would be useful. (And as it was said above, none of these are super secrets anyways but rather standards stats most webpages and apis already collecting).
Another side effect would be to know which platforms to tune the docs and tutorial/workshop material as users don't necessarily communicate issues upstream.

@msdemlei
Copy link
Contributor

msdemlei commented Jul 10, 2022 via email

@at88mph
Copy link
Contributor Author

at88mph commented Jul 11, 2022

Hi @msdemlei. The new format would just append (%OS_NAME). If that doesn't conform to anything in that document we can rework the layout. The examples (https://ivoa.net/documents/Notes/softid/20210115/NOTE-softid-1.0-20210115.html#tth_sEc2.3) suggest that wrapping it in parentheses may not be necessary though. Do you have a strong opinion on that?

@msdemlei
Copy link
Contributor

msdemlei commented Jul 13, 2022 via email

@at88mph
Copy link
Contributor Author

at88mph commented Aug 8, 2022

According to https://www.rfc-editor.org/rfc/rfc7230#section-3.2.6:

A string of text is parsed as a single value if it is quoted using
double-quote marks.

I'm pretty sure it's referring to the entire value though, not just the OS name appendage.

Mozilla has examples (https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/User-Agent):

Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko)
Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/91.0.4472.124 Safari/537.36 Edg/91.0.864.59

There are no quotes around the OS names here, and there are parentheses.

RFC 7231 and 7230 succeed 2616, and also states:

User-Agent = product *( RWS ( product / comment ) )

Where the product is a name "/" version, but makes no mention of double-quoting it, just that there is required white space. See also https://www.rfc-editor.org/rfc/rfc7230#section-3.2.

I'm happy to remove the parentheses, but it would also be beneficial to remain consistent with what may be expected. Thoughts?

@bsipocz bsipocz changed the title Fixes #343 ENH: Add operating system to user-agent Aug 8, 2022
@msdemlei
Copy link
Contributor

msdemlei commented Aug 15, 2022 via email

@bsipocz bsipocz added this to the v1.4 milestone Aug 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants