Parse new padding-counts lines

As found in today's CollecTor logs:

2017-05-10 06:09:07,711 WARN o.t.c.b.SanitizedBridgesWriter:1212 Unrecognized line 'padding-counts 2017-05-10 01:48:43 (86400 s) bin-size=10000 write-drop=10000 write-pad=10000 write-total=10000 read-drop=10000 read-pad=10000 read-total=70000 enabled-read-pad=0 enabled-read-total=0 enabled-write-pad=0 enabled-write-total=0 max-chanpad-timers=0'. Skipping.

We should provide support for parsing these lines in relay descriptors and in bridge descriptors if we keep those lines there (#22216 (moved)).

Optimistically aiming for 1.7.0.

changed milestone to %metrics-lib 1.7.0

added component::metrics/library milestone::metrics-lib 1.7.0 owner::karsten priority::medium resolution::implemented severity::normal status::closed type::enhancement labels

I have a patch, but I want to take another look before putting it here for review. Just saying so that nobody else starts writing a patch now.

Trac:
Owner: metrics-team to karsten
Status: new to accepted

Please find this commit in my task-22217 branch. I hear Samdney would like to take this one. Samdney, whatever you find, please just comment on this ticket and write down your thoughts or questions. Thanks!

Trac:
Cc: N/A to Samdney
Status: accepted to needs_review

Samdney says off-Trac that she'd rather leave this review to iwakeh. Thanks anyway!

iwakeh, please also find the two tweaks in a subsequent commit in the same branch.

Please find four commits in my repo branch.

Open topics I noticed in other parts of the code: The empty key issue seems to also apply to comma separated key-value pairs and maybe others. Similarly repeated keys. New ticket?

It would be good to add the check for the correct exception string also to other test methods in ExtraInfoDescriptorTest. Another ticket?

Trac:
Status: needs_review to needs_revision

Replying to iwakeh:

Please find four commits in my repo branch.

Looks good. Merged into mine together with a few whitespaces fixes and after prefixing instance method calls and references to instance variables with this. to be consistent with the other code.

Open topics I noticed in other parts of the code: The empty key issue seems to also apply to comma separated key-value pairs and maybe others. Similarly repeated keys. New ticket?

I included a fix and tests for empty keys. I briefly pondered adding checks and tests for duplicate keys, but I'd rather want to postpone that discussion in order not to de-stabilize master this close to the release. Maybe we can combine that with a minor code cleanup where we're duplicating less of that parsing code. Want to create that new ticket?

It would be good to add the check for the correct exception string also to other test methods in ExtraInfoDescriptorTest. Another ticket?

Yes, new ticket sounds good. Mind creating one?

If you like these changes, I'll merge that branch into master and start preparing the release. Thanks!

Trac:
Status: needs_revision to needs_review

Replying to karsten:

Replying to iwakeh:

Please find four commits in my repo branch.

Looks good. Merged into mine together with a few whitespaces fixes and after prefixing instance method calls and references to instance variables with this. to be consistent with the other code.

Thanks!

Open topics I noticed in other parts of the code: The empty key issue seems to also apply to comma separated key-value pairs and maybe others. Similarly repeated keys. New ticket?

I included a fix and tests for empty keys. I briefly pondered adding checks and tests for duplicate keys, but I'd rather want to postpone that discussion in order not to de-stabilize master this close to the release. Maybe we can combine that with a minor code cleanup where we're duplicating less of that parsing code. Want to create that new ticket?

It would be good to add the check for the correct exception string also to other test methods in ExtraInfoDescriptorTest. Another ticket?

Yes, new ticket sounds good. Mind creating one?

I'll create the new tickets, sure.

If you like these changes, I'll merge that branch into master and start preparing the release. Thanks!

Looks fine; checks and tests pass. => merge ready.

Trac:
Status: needs_review to merge_ready

Added tickets #22279 (moved) and #22280 (moved).

Great, thanks! Cherry-picked/squashed, and pushed to master. Closing!

Trac:
Status: merge_ready to closed
Resolution: N/A to implemented

closed

mentioned in issue #22279 (moved)

mentioned in issue #22280 (moved)