#22983: Add a Descriptor subinterface and implementation for Tor web server logs -----------------------------+----------------------------------- Reporter: iwakeh | Owner: metrics-team Type: enhancement | Status: needs_revision Priority: Medium | Milestone: metrics-lib 2.2.0 Component: Metrics/Library | Version: Severity: Normal | Resolution: Keywords: metrics-2017 | Actual Points: Parent ID: | Points: Reviewer: | Sponsor: -----------------------------+-----------------------------------
Comment (by karsten): Heh, that's indeed a huge comment! And this one is going to be huge, too. However, rather than splitting it into multiple smaller comment I'll try to keep it as organized as I can by using the output of `git log --reverse --pretty=oneline origin/master..iwakeh/task-22983-4` as section headers in this ~~little essay~~comment. ==== 1232ff767375993f65a4e78d3a9383b23681925b Added a space. This was commit c6c805c before your rebase to master. In the future, please avoid rebasing the branch under review if you can, because it changes all commit IDs and makes it harder to refer to commits here. In most cases I'll be able to do the rebase as part of merging to master just fine. Other than that, the explanation in your previous comment makes sense to me and this commit is good to go in. ==== 4434316a769ea6403d2872073bc76f10a2822cf2 Added package-info. This was commit 3b426bf before your rebase to master. We agree on this one plus the fixup commit below, so it's good to go in. ==== 04997b1d9b3a73d2724a2e9d39a144895ded460f Add new descriptor type for web server access logs. This was commit 7822a27 before your rebase to master. It's the commit that most of the subsequent fixup and squash commits will be squashed into. I'll comment on your feedback under the fixup commits below. ==== 444b55709bef99b1f63a20e3595c1439aaa68775 Add tests for log descriptor functionality. This was commit 929e265 before your rebase to master. This one still needs review. That review was so far blocking on my question regarding validation vs. sanitization. I'll get to this in the next round after that question is resolved. So, still '''needs review'''! ==== 4b3f27e93761b172dd3ee102f89c66059bd3da7a squash! Added package-info. This was commit 1bb7a43 before your rebase to master. You say that you agree with these tweaks, so I'll take that as a go. ==== be986cc88e66cb1df9772e65421462ca0570e190 fixup! Add new descriptor type for web server access logs. This was commit e4d6e90 before your rebase to master. You say that you're fine with these changes and have more tweaks in your branch. I'll assume this one is good to be merged then, and I'll comment on your subsequent commits further down below. ==== 3de717268c3d27ad096cf669fc73a52f049acb59 fixup! Add new descriptor type for web server access logs. This was commit 664f540 before your rebase to master. This commit brought up a few topics that we need to resolve before moving on. ===== Variable names Thanks for writing down your thinking about variable names in the given detail. It helps a lot, not only for this ticket but also as a guide for future tickets. Let's go through the examples: - Leaving out the somewhat redundant "descriptor" from `rawDescriptorBytes` and `descriptorFile` is fine with me. It's indeed obvious what's meant here. - I quite strongly disagree with `isValid(line)` as a name for a method that takes a line, tries to validate it, and returns whether that was successful. To me, `isX()` is the name for a getter, not the name for a method that does something with a given parameter. If this were a `Line` class with a method `isValid()` that returns whether the line is valid or not, that would be something different. But that's not the case here. For another example, consider `File.delete()` which deletes the file and returns whether that was successful. We wouldn't argue about renaming that method to `isDeleted()` just because it returns `true` or `false`. As a general rule I'd say that the name of a method that performs something should be the verb describing the action, whereas `is` is typically reserved for getters. Ah, and in this case it's up to the documentation to say what `validate(line)` returns, though that's relatively obvious. - Leaving out "line" in `sanitizeLine(line)` and friends is okay, too. - You're probably right about keeping `logBytes` rather than `rawDescriptorBytes`. ===== `LogDescriptor` interface I agree with keeping `LogDescriptor`. But let me explain my earlier hesitation: - I'm not a big fan of marker interfaces. :) However, it seems that we'll soon add a `LogDescriptor.LogLine` subinterface, and that is (to me) a good reason to keep the `LogDescriptor` interface. - In the future we might create an `AnnotatedDescriptor` interface with a `getAnnotations()` method, a `RawDescriptor` interface with a `getRawDescriptorBytes()` and a `getRawDescriptorLength()` method, and a `ParsedDescriptor` interface with a `getUnrecognizedLines()` method, and a `ReadDescriptor` interface with a `getDescriptorFile()` method. And then we'll turn `Descriptor` into a marker interface! :) And `LogDescriptor` would only extend maybe `ParsedDescriptor` and `ReadDescriptor`. And we'd create a `TorDescriptor` interface for all relay and bridge descriptors and a `RelayDescriptor` and `BridgeDescriptor` for the various types of relay and bridge descriptors. And so on. Not part of this ticket, obviously. But it seems okay to start going in this direction now with a `LogDescriptor` interface. ===== Validation vs. sanitization I'm still confused what validation means in this context. Is a line containing a POST request a valid line, or one that uses FTP as protocol or that returns HTTP 404 as status code. It's okay that CollecTor skips these lines as part of the sanitizing process. But that doesn't make them invalid. I'm also a bit uncleear if the separate validate and sanitize steps have a negative impact on performance. In theory, it should be sufficient to touch each line once. But I could be convinced that we're trading performance for better design, if this is the case. However, I'd really want us to be clear what it means for a line to be valid! ==== e8a2d57b3fad2fdb94d98ba472314fd2a0fb0ada fixup! fixup! Add new descriptor type for web server access logs. This one is fine. ==== 3864d14c8491c6a76514a28c702c3da5d1eb5775 fixup! fixup! Add new descriptor type for web server access logs. This one, too. ==== 3e3dd56e317ef61296261291dc3ee29d42ebf3b8 Make code more readable by renaming method names. The commit message does not indicate this, so I'll ask: Is this commit and are subsequent commits supposed to be squashed with 04997b1? See my ~~rant~~remarks about `isValid` from above. That's something I'd like us to change. ==== 5b20be449785b1e28fd10bcb814e697cb9de178d Rename rawDescriptorBytes to logBytes. Okay. One thing we should do is document whether `private byte[] logBytes` might be compressed or not. We have been discussing that many times now, and I'm deeply confused already what we're doing there. ==== f5456c474f6a5a9683c6cf6ba3f791bc5d6278ec Avoid missleading name for variable. Sounds good. ==== 629ef152be1fd2f5a00d203b614fc01e946c518d Tweak pattern for logline validation. One question about the pattern: Does the `?:` in `"^((?:\\d{1,3}\\.){3}\\d{1,3}) ..."` mean that we're now ignoring the first 3 octets regardless of whether they're `0.0.0.` or not? That would not follow the specification then where we claim that ''"If the remote hostname starts with "0.0.0.", it is kept unchanged, otherwise it's rewritten to "0.0.0.0"."'' Example: `1.2.3.4` would be rewritten to `0.0.0.4` if we simply ignore the first three octets, rather than `0.0.0.0`. In a way, I like that approach even better, but we'd have to change the spec for that. And we should both be certain that this is what we want to do. ==== e24dda11613f340da3fbd6f1a93ba07d857f0b16 Remove getLogDateMillis and move getLogDate to WebServerAccessLog. I wonder why there's no `getLogDateMillis()` anymore. But I can be convinced that users should just extract millisecond from `LocalDate` if they need them. Is that the plan? If so, green light. ==== 6a6e93a2fd8b3f3b912ce9a258f4b32069c18ef8 (iwakeh/task-22983-4) Set dev version. Let's revert this commit. It bumps to the wrong dev version (2.2.0-dev rather than 2.1.1-dev), and it's something we should do as part of merging to master. In fact, I bumped to 2.1.1-dev this morning. So, this commit can go away, by reverting it, not by simply dropping it. ==== Closing remarks Whee! This was hard work. And we're not done yet. But we're getting closer. :) Thanks for working on this! -- Ticket URL: <https://trac.torproject.org/projects/tor/ticket/22983#comment:47> Tor Bug Tracker & Wiki <https://trac.torproject.org/> The Tor Project: anonymity online
_______________________________________________ tor-bugs mailing list tor-bugs@lists.torproject.org https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs