patch for missing torperf annotaion

see attached patch for the bug reported by Thomas

added component::metrics/library owner::karsten priority::medium resolution::fixed severity::normal status::closed type::defect labels

Trac:
0001-Prevent-disappearance-of-torperf-annotation.patch

patch

The patch throws a NullPointerException

Exception in thread "main" java.lang.NullPointerException at org.torproject.descriptor.impl.TorperfResultImpl.getDataPercentiles(TorperfResultImpl.java:434) at mteam.ConvertToJson$JsonTorperfResult.convert(ConvertToJson.java:1315) at mteam.ConvertToJson.main(ConvertToJson.java:105) at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:39) at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25) at java.lang.reflect.Method.invoke(Method.java:597) at com.intellij.rt.execution.application.AppMain.main(AppMain.java:144)

Trac:
Username: tomlurge

Trac:
Username: tomlurge
Cc: N/A to tomlurge

Trac:
0002-Don-t-skip-percentiles-after-annotation.patch

patch 2

Thanks for testing and reporting!

Attached the patch for the npe. I also extended the test.

Patch 2 works partially. It adds the correct descriptor type to the first descriptor in a file. All the following descriptors still have a descriptor-type "null".

Trac:
Username: tomlurge

Trac:
0003-More-tests-for-documenting-intended-behavior.patch

only additional tests

This is a feature not a bug. There was only an annotation for the first entry.

@karsten: can you confirm that this is the intended behavior of metrics-lib?

I attached a patch that extends only the tests and makes the feature explicit.

Trac:
Status: new to needs_information

Thinking of it: iwakeh might be right, and that would mean that we also need to distill 'date' and 'server name' (and more?) from the file name and make all entries objects in an array. Please check docs/data.md (the very end of it) if the JSON serialization makes sense or if it should be modified as sketched out here. I didn't have a spec for Torperf, I might very well have gotten this just plain wrong.

Trac:
Username: tomlurge

The patches look good, though I'd want to implement the fix slightly different. I just pushed branch task-17696 to my public repository which contains iwakeh's patches, some cleanups, and a slightly different implementation that produces the same result. If you like it, I'll squash and merge that. If not, please provide a patch based on my branch or tell me which of my subsequent commits you dislike.

Regarding the question above whether there should be an annotation for each line containing measurements, I think I agree with iwakeh. metrics-lib does the right thing here, with this patch. It's CollecTor that is broken which should put in a @type annotation for each measurement line. But I'll consult atagar who typically helps with discussions about @type annotations and Rob Jansen who works on a successor of Torperf and who just recently started producing @type torperf 1.0 files. This should become a new ticket then, either for metrics-lib if I'm wrong here, or for CollecTor if I'm right.

Trac:
Status: needs_information to needs_review

Your changes are ok for being added to the first release. I usually prefer avoiding changes to method input parameters, but I'm not religious about it.

Sounds like a reasonable rule not to change method input parameters. Changed back. And merged to master. Thanks!

I'll also create a CollecTor ticket to add the missing @type annotations. But that doesn't affect this bug being fixed now, I think. Resolving.

Trac:
Status: needs_review to closed
Resolution: N/A to fixed

closed