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

feat #6: Track number of requests per user agent #11

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

MarcusRiemer
Copy link

This is a first example on how working with one large table could look like when tracking user agents. Notable things from the top of my head:

  • I somewhat attempted to make the new columns optional. If someone doesn't run the migration but updates to this version, everything should work as it did before. But I wouldn't know how this could be properly tested.
  • Making user agent tracking optional was a little bit annoying for the Redis queuing that I missed initially, as I don't use it. I took the liberty to refactor the serialization and deserialization of such keys but didn't include any migration path because I have no idea how to test this properly. So users would need to drain the queue as part of the migration. I guess this could be salvaged by checking whether the key is in the old format if this feels important.
  • Customization of extracted strings is hinted at, but not yet implemented. But this should be mainly a documentation issue, as the code is technically only missing a setter.
  • I bumped the minimum Rails version to 6 in order to use SQL upserts. This should be more performant and more secure (for whatever security is needed to track single visits, but hey). This however has an important implication: As in SQL NULL is not equal to NULL the upsert will only work if all columns have empty strings as default. This is not yet reflected in the migration.
  • I didn't touch the UI for the moment.

device_types[row[2].to_sym] += total_group if row[2]
operating_systems[row[3].to_sym] += total_group if row[3]
total_sum += total_group
end
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The aggregation has to live in Ruby for the moment because I didn't want to hard code which exact values for browser, operating_system and device_type we want to allow. Technically this would be currently know from my implementation of user_agent_extractors.

But it didn't feel right to add / generate mildly complex SQL to aggregate exact values when not knowing whether this might just be good enough.

date: Date.today,
def self.user_agent_extractors
@@user_agent_extractors ||= {
browser: lambda do |b|
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a limitation to use browser.name, browser.device.name and browser.platform.name ?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, this was just me wanting to make 100% sure I know all the possible values that could be written. Just as you mentioned (#6 (comment)): The potential for far too many rows is big.

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