Radius decoder

[wg]

Thanks! Looks pretty good, though I haven't had a chance to do a detailed review yet.

I'm not certain, but suspect that we can't just use 5 for APP_PROTOCOL because the column has been overloaded to mark other flow types. You'll need to ask around about how IDs are being allocated for that, hopefully it's not just an autoincrement that's environment specific =/

A minor nit, but is the _A_ in the RADIUS column names necessary? Pretty much everything in RADIUS seems to be an attribute so feels a little ugly and redundant ;-)

[theclapp]

@wg said:

we can't just use 5 for APP_PROTOCOL because the column has been overloaded to mark other flow types

That's correct. app_protocol 5 is snmp, 6 is streaming telemetry. Other values are currently defined in kentik/sql_schema/sql/migrate/kentik-1562795815-2019-07-10-protocol-fixes.sql. The next available one is 9, so far as I can tell. You might need a migration to define that, though.

[i3149]

ch_www=> select * from mn_lookup_app_protocol; id | display_name | add_to_standard_fields | status | metadata | app_protocol ----+---------------------+------------------------+--------+---------------------------------------+-------------- 1 | DNS | t | A | {"group_name": "Application Decodes"} | dns 2 | HTTP | t | A | {"group_name": "Application Decodes"} | http 3 | TLS | t | A | {"group_name": "Application Decodes"} | tls 4 | DHCP | t | A | {"group_name": "Application Decodes"} | dhcp 5 | SNMP | f | D | {"group_name": "Application Decodes"} | snmp 7 | K/V | f | D | {"group_name": "Application Decodes"} | kv 8 | Synthetic | f | A | {"group_name": "Device Metrics"} | synthetic 6 | Streaming Telemetry | f | A | {"group_name": "Device Metrics"} | st (8 rows)

Is how I look it up.

As Larry said, you need to make a sql migrate to add 9 to this table. Then 9 is yours and can be hard coded.

One to copy is sql/migrate/kentik-1562188106-2019-07-03-gnmi_changes_2.sql.

On Wed, Jul 17, 2019 at 8:14 AM Larry Clapp notifications@github.com wrote:

@wg https://github.com/wg said:

we can't just use 5 for APP_PROTOCOL because the column has been overloaded to mark other flow types

That's correct. app_protocol 5 is snmp, 6 is streaming telemetry. Other values are currently defined in kentik/sql_schema/sql/migrate/kentik-1562795815-2019-07-10-protocol-fixes.sql https://github.com/kentik/sql_schema/blob/e1eb0882b7bae9e074e3a0e9c715fce28d7f064a/sql/migrate/kentik-1562795815-2019-07-10-protocol-fixes.sql. The next available one is 9, so far as I can tell. You might need a migration to define that, though.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/kentik/kprobe/pull/4?email_source=notifications&email_token=AACNARF4FPCCE4DDUMCP7ELP74ZPDA5CNFSM4IEB3NJKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD2EUO5Y#issuecomment-512313207, or mute the thread https://github.com/notifications/unsubscribe-auth/AACNARHSLROOMESI2ZJXDZ3P74ZPDANCNFSM4IEB3NJA .

[tomaszjonak]

I'll change that. Got a bit sidetracked by alert-api stuff for pandora and forgot to check numbering in DB. I'm gonna write relevant migration soon(tm) and allocate app_protocol id as well as custom column mappings.

[tomaszjonak]

App protocol columns deployed. Slight bump.

[wg]

I'm happy enough with the changes, I'd say go ahead and merge. Next time I'd like to see a little more meaningful commit messages though =) Messages like "Attributes added" and "Review tweaks" aren't very helpful when looking through the commit log, particularly since they're no longer attached to the PR.

My usual process for testing kprobe is to make a x86_64-unknown-linux-musl release build and check that binary in as kprobe-dev in the operations repo. Puppet will automatically deploy that to all prod nodes and then you can let it bake for a while and check that everything still looks sane. Want to give it a try?