Opened 2 years ago

Closed 2 years ago

#22217 closed enhancement (implemented)

Parse new padding-counts lines

Reported by: karsten Owned by: karsten
Priority: Medium Milestone: metrics-lib 1.7.0
Component: Metrics/Library Version:
Severity: Normal Keywords:
Cc: Samdney Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

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).

Optimistically aiming for 1.7.0.

Child Tickets

Change History (9)

comment:1 Changed 2 years ago by karsten

Owner: changed from metrics-team to karsten
Status: newaccepted

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.

comment:2 Changed 2 years ago by karsten

Cc: Samdney added
Status: acceptedneeds_review

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!

comment:3 Changed 2 years ago by karsten

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.

comment:4 Changed 2 years ago by iwakeh

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?

comment:5 Changed 2 years ago by iwakeh

Status: needs_reviewneeds_revision

comment:6 in reply to:  4 ; Changed 2 years ago by karsten

Status: needs_revisionneeds_review

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!

comment:7 in reply to:  6 Changed 2 years ago by iwakeh

Status: needs_reviewmerge_ready

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.

comment:8 Changed 2 years ago by iwakeh

Added tickets #22279 and #22280.

comment:9 Changed 2 years ago by karsten

Resolution: implemented
Status: merge_readyclosed

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

Note: See TracTickets for help on using tickets.