Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

draft public protos #414

Closed
wants to merge 1 commit into from
Closed

draft public protos #414

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
74 changes: 74 additions & 0 deletions src/service/cdr.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,74 @@
syntax = "proto3";

package helium.public_cdr;

enum public_cdr_type {
wifi = 0;
cbrs = 1;
}

enum public_record_type {
data = 0;
international_roaming_data = 1;
connection = 2;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aren’t connections prerequisites of any other kind of record? Could counting these result in inflated numbers if a consumer were under the impression a “connection” were distinct from a data session?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Connections and sessions are related but provide two distinct offerings to downstream services. They are also published at different cadences, with connections at a much faster cadence. We may or may not want to include connections in the public bucket but it is included for now as a 1-1 match for our internal feeds

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i agree it's valid to output both connections and "sessions" but it's redundant to have a public_cdr_connection_v1 which would logically always have a RecordType: PublicRecordType::Connection; the two "report" messages should be session_v1 and connection_v1 and then this record_type enum should be limited to use in the "session" messages with variants like "data", "international_roaming_data", etc. should we also account for "voice" and "text" traffic or is that not differentiated when being processed by hotspots?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree, no need for RecordType in the connection proto. At this time I as I understand it we are only interested in outputting data record types

}

enum public_traffic_source {
helium_mobile = 0;
other = 1;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any option for an “other” right now? We should avoid this catch all until we know we need it. If it’s for an “unknown” it should probably be the first option since that feels like a safer default value than attributing to any specific carrier/provider

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other is currently in use as a placeholder until a confirmed list of sources is provided

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we need a placeholder when adding variants to a protobuf enum is fully backward compatible?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is a draft, its there as a placeholder until things are confirmed and should we end up omitting a source, then it will be designated as other

}

message public_cdr_report_v1 {
uint64 received_timestamp = 1;
public_cdr_v1 report = 2;
}

message public_cdr_v1 {
// id of the cdr
string id = 1;
// type of the cdr
public_cdr_type cdr_type = 2;
// the record type of the cdr
public_record_type record_type = 3;
// origin of the cdr
public_traffic_source traffic_source = 4;
// the hotspot serial number
string agw_sn = 5;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agw_sn probably too specific to helium mobile. hotspot_sn?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i also wonder if this is an opportunity to start distancing ourselves from the cruft of the "split identity" that a cbrs radio/hotspot pair comes with by collapsing the gateway_sn and radio_sn fields into a single hotspot_sn field which would have a oneof type and then the two variants of the oneof would be something like:

message radio_identity {
    string gateway_sn = 1;
    string radio_sn = 2;
}

message session_v1 {
    string id = 1;
    string pubkey = 2;
    oneof hotspot_id {
        string unified = 3;
        radio_identity split = 4;
    }
}

which would have some benefits when decoding/parsing the message into a language-native data type to immediately determine the type of hotspot/radio combo and avoid the need for things like handling options of strings in rust for the 99% of hotspots producing reports

// the public key of the hotspot
string hotspot_pubkey = 6;
// the serial number of a cbrs radio
string radio_serial = 7;
// the cbsd_id of a cbrs radio
string cbsd_id = 8;
uint64 total_volume = 9;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we please add some comment indicating the units for each type of usage volume? I believe these are all in bytes?

uint64 uplink_volume = 10;
uint64 downlink_volume = 11;
// timestamp in millisecs of when the record was created in the network
uint64 timestamp = 12;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Optionally rename this to timestamp_ms? Could do similar for any other type which has a unit, so received_timestamp -> received_timestamp_ms etc would also be nice.

}

message public_cdr_connection_report_v1 {
uint64 received_timestamp = 1;
public_cdr_connection_v1 report = 2;
}

message public_cdr_connection_v1 {
// id of the cdr
string id = 1;
// type of the cdr
public_cdr_type cdr_type = 2;
// the record type of the cdr
public_record_type record_type = 3;
// origin of the cdr
public_traffic_source traffic_source = 4;
// the hotspot serial number
string agw_sn = 5;
// the public key of the hotspot
string hotspot_pubkey = 6;
// the cbsd_id of a cbrs radio
string cbsd_id = 7;
// the serial number of a cbrs radio
string radio_serial = 8;
// timestamp in millisecs of the connection event
uint64 timestamp = 9;
}
Loading