[Author Prev][Author Next][Thread Prev][Thread Next][Author Index][Thread Index]
Re: [tor-bugs] #21236 [Metrics/Metrics website]: Put a visualization of Tor Browser downloads and updates on the Metrics website
#21236: Put a visualization of Tor Browser downloads and updates on the Metrics
website
-------------------------------------+------------------------------
Reporter: karsten | Owner: karsten
Type: enhancement | Status: needs_review
Priority: High | Milestone:
Component: Metrics/Metrics website | Version:
Severity: Normal | Resolution:
Keywords: | Actual Points:
Parent ID: | Points:
Reviewer: | Sponsor:
-------------------------------------+------------------------------
Comment (by karsten):
Thanks for the quick response! And thanks a lot for making those build
improvements.
I made some tweaks to your commits and pushed them to
[https://gitweb.torproject.org/karsten/metrics-web.git/log/?h=task-21236-2
my task-21236-2 branch]. I'd like to squash that commit and yours into my
initial commit 597e715 at the end, if you don't mind. Unless there's a
reason to keep them separate, in which case, please let me know.
But before we squash or don't squash anything, let's talk about the
`Database` class. I like the idea of encapsulating database access---in
theory. I even had a similar class like yours a day or two ago. But I
took that out again, because I felt it only added complexity for no
particular gain. Let me elaborate.
The `Main` class still imports types from `java.sql`, meaning that the
`Database` class does not hide that we're talking to an SQL database via
JDBC. And the methods in `Database` all contain just a single line of
code or two, meaning that we're not really hiding much complexity. The
reason is that JDBC is already an interface hiding away all the complexity
that comes with talking to an SQL database.
So, let's talk about the purpose of having this separate `Database` class.
I assume the main reason for having it is testing. Is it going to be
easier to test the `Database` class than to test the single methods in
modules that are related to database access? And maybe even more
importantly, is it easier to mock the `Database` class than to provide a
dummy database via a custom JDBC string and just test the methods
contained in modules that way? If the answers are "yes and yes", would
you mind writing a simple test for the new `Database` class, just to get
us started?
Otherwise, would you mind if we postpone this change and keep all database
methods in the `Main` class for now? It's easy to extract database
functionality (and downloading functionality or .csv-file writing
functionality) as part of the restructuring. Don't get me wrong, I'm a
supporter of encapsulation if something is fully encapsulated and for a
good reason. But I'm also a fan of self-contained and therefore often
more readable code. (For example, do you think we'll remember in a year
when we write the next module that we disable auto-commit when connecting
to the database?!)
And here are some minor points that need discussion:
- If we keep the `Database` class, we should either move it to the
`org.torproject.metrics.webstats` package, or start a `shared/src/`
directory for code that is supposed to be shared between modules. My
favorite would be to start with the former and consider doing the latter
once we have a second module using it.
- Should the arguments `--filter` and `--db` be switched around, so that
people can specify their own database but omit the filter? Alternatively,
we might want to say more clearly how to pass an argument for `--filter`
that doesn't filter anything.
- I didn't run this code yet. Does `writeStatistics()` add a newline
after writing the header?
- As you point out, we should write tests. I'm happy to do that, though
I'm afraid the most interesting parts require mocking either the web
server or the database. If you have suggestions for doing that, please
let me know. Otherwise I'd start with some simple tests for
`parseLogLines()` or `writeStatistics()`.
And here's another remaining issue from above, listed for completeness:
- I didn't find time to fix yet is that locale detection is not always as
precise as it could be. Example from the .csv file:
`2017-01-10,tmup,o,,0.3.0b1-0.3.0b2-linux32-en-US.xml,,3`
Thanks again!
--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/21236#comment:7>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online
_______________________________________________
tor-bugs mailing list
tor-bugs@xxxxxxxxxxxxxxxxxxxx
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs