feat: refactor tables to reuse common fields #36

adklempner · 2024-08-07T04:19:00Z

Depends on: #44

Adds a device type field for all metrics
Creates a new table for fields that are common across metrics
Removes fields from above in existing tables, adds a foreign key to above
Default to SQL time for createdAt
Updates types and put/process functions to work with new table and foreign key

igor-sirotin · 2024-08-07T12:31:01Z

telemetry/sql/000014_device_type.up.sql

Same here, is it possible to introduce some RecordID to each record stored in separate tables, and then introduce a new table that will hold all common fields? And just LEFT JOIN the tables on read.

Obviously, it's a big change, and not required for this PR.
Yet, if we keep adding more fields, would be nice to consider.

Same here, is it possible to introduce some RecordID to each record stored in separate tables, and then introduce a new table that will hold all common fields? And just LEFT JOIN the tables on read.

Would this mean we have to write to two tables each time we receive data?

Yes, but I don't think that's a problem. Is it?

Added in latest commit

igor-sirotin

Thanks for the improvements!
I don't have any strong objections, mostly suggestions.
Also, I would recommend to keep this PR only about adding deviceType, and open a separate PR for the commonFields. It would help to to block one change with the other one. And the review would be simpler for the folks :)

igor-sirotin · 2024-08-15T10:03:43Z

pkg/types/types.go

 }

 type ErrorSendingEnvelope struct {
-	CreatedAt    int64        `json:"createdAt"`
+	Id           int          `json:"id"`


Suggested change

Id int `json:"id"`

ID int `json:"id"`

igor-sirotin · 2024-08-15T10:06:09Z

telemetry/common_fields.go

+	"github.com/status-im/telemetry/pkg/types"
+)
+
+func InsertCommonFields(db *sql.DB, data types.CommonFieldsAccessor) (int, error) {


Why not just pass pointer to CommonFields here? I think using interface here is a bit overcomplicating.

Suggested change

func InsertCommonFields(db *sql.DB, data types.CommonFieldsAccessor) (int, error) {

func InsertCommonFields(db *sql.DB, data *types.CommonFields) (int, error) {

Ah I didn't know it could work like this! much simpler, thanks

igor-sirotin · 2024-08-15T10:11:32Z

telemetry/sql/000014_device_type.up.sql

+ALTER TABLE peercount ADD COLUMN deviceType VARCHAR(255);
+ALTER TABLE receivedMessages ADD COLUMN deviceType VARCHAR(255);
+ALTER TABLE receivedEnvelopes ADD COLUMN deviceType VARCHAR(255);
+ALTER TABLE sentEnvelopes ADD COLUMN deviceType VARCHAR(255);
+ALTER TABLE errorSendingEnvelope ADD COLUMN deviceType VARCHAR(255);
+ALTER TABLE peerConnFailure ADD COLUMN deviceType VARCHAR(255);


Do we still need to add deviceType to all tables?

Keeping it in as it's included in separate PR that will be merged first #44

igor-sirotin · 2024-08-15T10:14:06Z

telemetry/sql/000015_common_fields.up.sql

+BEGIN
+
+    -- Add the new column for the foreign key
+    EXECUTE format('ALTER TABLE %I ADD COLUMN IF NOT EXISTS commonFieldsId INTEGER', table_name);


Not pushing on terms here, just sharing opniong.

I would call this column just id. And the commonFields, would simply be TelemetryRecord. And then there're difference extensions of these records that reference the main record by ID.

Suggested change

EXECUTE format('ALTER TABLE %I ADD COLUMN IF NOT EXISTS commonFieldsId INTEGER', table_name);

EXECUTE format('ALTER TABLE %I ADD COLUMN IF NOT EXISTS id INTEGER', table_name);

Do you mean multiple rows in other tables sharing the same TelemetryRecord? e.g. a row in receivedMessages and a row in peercount with the same value in column id?

I think this would work but would require removing the timestamp from TelemetryRecord and putting it back into the other tables.

Nah, never mind. After second thought on this, I decided to google how to do it right 😄
What we're doing here is called Class Table Inheritance.

So my suggestion would be just to rename CommonFields to TelemetryRecord (or simply Record).
And consequently rename commonFieldsId -> record_id.

But this is not just my subjective opinion, leaving this at your decision ofc.

Renamed in latest commit

igor-sirotin · 2024-08-15T10:16:37Z

telemetry/sql/000015_common_fields.up.sql

+            RAISE NOTICE 'Foreign key constraint already exists on %', table_name;
+    END;
+
+    -- Remove columns that are now in commonFields


Any chance to migrate current telemetry to the new table as well?

igor-sirotin · 2024-08-15T10:16:55Z

telemetry/receivedmessage.go

+	err := db.QueryRow(`
+		SELECT COUNT(*) 
+		FROM receivedMessages rm
+		JOIN commonFields cf ON rm.id = cf.id


Looks like it should be rm.commonFieldsId

igor-sirotin · 2024-08-15T10:20:31Z

telemetry/receivedmessage.go

+	err = db.QueryRow(`
+		SELECT COUNT(*)
+		FROM receivedMessages rm
+		JOIN commonFields cf ON rm.id = cf.id


Looks like it should be rm.commonFieldsId

igor-sirotin · 2024-08-15T10:23:03Z

telemetry/receivedmessage.go

@@ -65,7 +67,7 @@ func (r *ReceivedMessage) put(db *sql.DB) error {
 }

 func queryReceivedMessagesBetween(db *sql.DB, startsAt time.Time, endsAt time.Time) ([]*types.ReceivedMessage, error) {
-	rows, err := db.Query(fmt.Sprintf("SELECT id, chatId, messageHash, messageId, receiverKeyUID, peerId, nodeName, sentAt, topic, messageType, messageSize, createdAt, pubSubTopic FROM receivedMessages WHERE sentAt BETWEEN %d and %d", startsAt.Unix(), endsAt.Unix()))
+	rows, err := db.Query(fmt.Sprintf("SELECT id, chatId, messageHash, messageId, receiverKeyUID, sentAt, topic, messageType, messageSize, pubSubTopic FROM receivedMessages WHERE sentAt BETWEEN %d and %d", startsAt.Unix(), endsAt.Unix()))


Shouldn't we LEFT JOIN commonFields here? 🤔

igor-sirotin · 2024-08-15T11:10:15Z

@richard-ramos please take a look as well

richard-ramos · 2024-08-15T13:22:57Z

pkg/types/types.go

+func (c CommonFields) GetNodeName() string {
+	return c.NodeName
+}
+
+func (c CommonFields) GetPeerID() string {
+	return c.PeerID
+}
+
+func (c CommonFields) GetStatusVersion() string {
+	return c.StatusVersion
+}
+
+func (c CommonFields) GetDeviceType() string {
+	return c.DeviceType
+}
+
+type CommonFieldsAccessor interface {
+	GetNodeName() string
+	GetPeerID() string
+	GetStatusVersion() string
+	GetDeviceType() string
+}


Having an interface for accesors might be an overkill.
Generally we'd use interfaces for common behavior, decoupling or restricting behavior, but looking at how it's being used in the code, you should be able to access the fields, (i.e. if you have a PeerCount, by calling myPeerCount.CommonFields). An argument could be made for decoupling the database code from the public facing structs, but, unless there's an intention/need to decouple these, we'll end up adding more complexity than required

richard-ramos · 2024-08-15T13:24:43Z

pkg/types/types.go

 	ID              int    `json:"id"`
 	MessageHash     string `json:"messageHash"`
 	SentAt          int64  `json:"sentAt"`
-	CreatedAt       int64  `json:"createdAt"`
 	PubsubTopic     string `json:"pubsubTopic"`
 	Topic           string `json:"topic"`
 	SenderKeyUID    string `json:"senderKeyUID"`


For consistency, should we use json:"senderKeyUID" or json:"senderKeyUid" . I see diff casing for uid in the json naming (like for example on line 65 and 96)

status-go uses UID for all json so I set it to that

I guess now it's too late to change it anyway, it would be a breaking change

richard-ramos · 2024-08-15T13:25:15Z

telemetry/aggregator_test.go

-	_, err = db.Exec("DROP TABLE IF EXISTS protocolStatsTotals")
-	if err != nil {
-		log.Fatalf("an error '%s' was not expected when dropping the table", err)
+	tables := []string{


I like this!

richard-ramos · 2024-08-15T13:28:37Z

telemetry/aggregator_test.go

-		log.Fatalf("an error '%s' was not expected when dropping the table", err)
-	}
+	for _, table := range tables {
+		_, err := db.Exec(fmt.Sprintf("DROP TABLE IF EXISTS %s", table))


I think it's possible to do this:

Suggested change

_, err := db.Exec(fmt.Sprintf("DROP TABLE IF EXISTS %s", table))

_, err := db.Exec("DROP TABLE IF EXISTS $1", table)

fun fact, turns out it only works with:

db.Exec(`DROP TABLE IF EXISTS $1`, table)

edit: nevermind, that doesn't work either. per gpt "In PostgreSQL, you can't use placeholders for table names or other identifiers."

Damn, that's annoying :(

richard-ramos · 2024-08-15T13:31:09Z

telemetry/peer_count.go

+	commonFieldsId, err := InsertCommonFields(db, r.data)
 	if err != nil {
 		return err
 	}

-	defer stmt.Close()
+	peerCountStmt, err := db.Prepare(`
+		INSERT INTO peerCount (common_fields_id, nodeKeyUid, peerCount)
+		VALUES ($1, $2, $3)
+		RETURNING id;
+	`)
+	if err != nil {
+		return err
+	}
+	defer peerCountStmt.Close()


Since both rows being inserte here are dependent on each other, we should use a transaction which should be passed to InsertCommonFields, and used in line 26 too (and maybe in line 37? not sure), with it's commit/rollback depending if there is an error.

richard-ramos · 2024-08-15T13:32:00Z

telemetry/peer_count.go

@@ -56,24 +58,24 @@ func (r *PeerConnFailure) process(db *sql.DB, errs *MetricErrors, data *types.Te
 		return err
 	}

-	stmt, err := db.Prepare("INSERT INTO peerConnFailure (timestamp, nodeName, nodeKeyUid, peerId, failedPeerId, failureCount, statusVersion, createdAt) VALUES ($1, $2, $3, $4, $5, $6, $7, $8) RETURNING id;")
+	commonFieldsId, err := InsertCommonFields(db, r.data)


See comment about using a trx

richard-ramos · 2024-08-15T13:32:09Z

telemetry/receivedenvelope.go

-							topic, receiverKeyUID, nodeName, processingError, statusVersion)
-							VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9)
+
+	commonFieldsId, err := InsertCommonFields(db, r.data)


See comment about using a trx

richard-ramos · 2024-08-15T13:32:23Z

telemetry/receivedenvelope.go

-							topic, senderKeyUID, peerId, nodeName, publishMethod, statusVersion)
-							VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10)
+
+	commonFieldsId, err := InsertCommonFields(db, r.data)


See comment about using a trx

richard-ramos · 2024-08-15T13:33:21Z

telemetry/receivedenvelope.go

-	stmt, err := db.Prepare(`INSERT INTO errorSendingEnvelope (messageHash, sentAt, createdAt, pubsubTopic,
-		topic, senderKeyUID, peerId, nodeName, publishMethod, statusVersion, error)
-		VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11)
+	commonFieldsId, err := InsertCommonFields(db, e.data.SentEnvelope)


See comment about using a trx

richard-ramos · 2024-08-15T13:33:41Z

telemetry/receivedmessage.go

@@ -35,27 +35,29 @@ func (r *ReceivedMessage) process(db *sql.DB, errs *MetricErrors, data *types.Te
 }

 func (r *ReceivedMessage) put(db *sql.DB) error {
-	stmt, err := db.Prepare("INSERT INTO receivedMessages (chatId, messageHash, messageId, receiverKeyUID, peerId, nodeName, sentAt, topic, messageType, messageSize, createdAt, pubSubTopic, statusVersion) VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11, $12, $13) RETURNING id;")
+	commonFieldsId, err := InsertCommonFields(db, r.data)


See comment about using a transaction

igor-sirotin · 2024-08-16T10:28:30Z

telemetry/peer_count.go

 		return err
 	}

-	stmt, err := db.Prepare("INSERT INTO peerCount (timestamp, nodeName, nodeKeyUid, peerId, peerCount, statusVersion, createdAt) VALUES ($1, $2, $3, $4, $5, $6, $7) RETURNING id;")
+	tx, err := db.BeginTx(context.Background(), nil)


Not a big deal, but it's better to actually pass some real context

richard-ramos

LGTM. Just left few nitpicks about things that probably werent introduced in this PR and might be worked on later!

richard-ramos · 2024-09-05T13:34:01Z

lib/common/test_utils.go

@@ -111,6 +89,11 @@ func DropTables(db *sql.DB) {
 		log.Fatalf("an error '%s' was not expected when dropping the table", err)
 	}

+	_, err = tx.Exec("DROP TABLE IF EXISTS telemetryRecord")


Should this table be included in the list on line 31?

It needs to be dropped after all other tables and indexes

richard-ramos · 2024-09-05T13:35:41Z

lib/metrics/common_fields.go

+	stmt, err := tx.Prepare(`
+		INSERT INTO telemetryRecord (nodeName, peerId, statusVersion, deviceType)
+		VALUES ($1, $2, $3, $4)
+		RETURNING id;
+	`)


As described in #43 consider using exec, or if this insertion is going to be called frequently, maybe it's a good idea to extract the prepared statement and make it available during the entire duration of the program.

richard-ramos · 2024-09-05T13:36:55Z

lib/metrics/peer_count.go

 		return err
 	}

-	stmt, err := db.Prepare("INSERT INTO peerCount (timestamp, nodeName, nodeKeyUid, peerId, peerCount, statusVersion, createdAt, deviceType) VALUES ($1, $2, $3, $4, $5, $6, $7, $8) RETURNING id;")
+	tx, err := db.BeginTx(context.Background(), nil)


Ideally the context should be passed down. Maybe this one could be used https://github.com/status-im/telemetry/blob/master/telemetry/server.go#L39 ?

richard-ramos · 2024-09-05T13:37:36Z

lib/metrics/peer_count.go

+	peerCountStmt, err := tx.Prepare(`
+		INSERT INTO peerCount (common_fields_id, nodeKeyUid, peerCount)
+		VALUES ($1, $2, $3)
+		RETURNING id;
+	`)


Consider using exec vs prepare #43

richard-ramos · 2024-09-05T13:38:00Z

lib/metrics/peer_count.go

+		return err
+	}
+
+	tx, err := db.BeginTx(context.Background(), nil)


Ideally the context should be passed down. Maybe this one could be used https://github.com/status-im/telemetry/blob/master/telemetry/server.go#L39 ?

richard-ramos · 2024-09-05T13:38:30Z

lib/metrics/peer_count.go

 		return err
 	}

-	stmt, err := db.Prepare("INSERT INTO peerConnFailure (timestamp, nodeName, nodeKeyUid, peerId, failedPeerId, failureCount, statusVersion, createdAt, deviceType) VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9) RETURNING id;")
+	stmt, err := db.Prepare("INSERT INTO peerConnFailure (common_fields_id, nodeKeyUid, failedPeerId, failureCount) VALUES ($1, $2, $3, $4) RETURNING id;")


See prev comment about using exec.

richard-ramos · 2024-09-05T13:38:54Z

lib/metrics/receivedenvelope.go

+		return err
+	}
+
+	stmt, err := tx.Prepare(`INSERT INTO receivedEnvelopes (recordId, messageHash, sentAt, pubsubTopic,


See prev comment about using exec.

richard-ramos · 2024-09-05T13:39:09Z

lib/metrics/receivedenvelope.go

+		return err
+	}
+
+	stmt, err := tx.Prepare(`INSERT INTO sentEnvelopes (recordId, messageHash, sentAt, pubsubTopic,


See prev comment about using exec.

richard-ramos · 2024-09-05T13:39:22Z

lib/metrics/receivedenvelope.go

-	stmt, err := db.Prepare(`INSERT INTO errorSendingEnvelope (messageHash, sentAt, createdAt, pubsubTopic,
-		topic, senderKeyUID, peerId, nodeName, publishMethod, statusVersion, error, deviceType)
-		VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11, $12)
+	tx, err := db.BeginTx(context.Background(), nil)


See comment about passing down context

richard-ramos · 2024-09-05T13:39:40Z

lib/metrics/receivedmessage.go

@@ -40,39 +41,53 @@ func (r *ReceivedMessage) Clean(db *sql.DB, before int64) (int64, error) {
 }

 func (r *ReceivedMessage) Put(db *sql.DB) error {
-	stmt, err := db.Prepare("INSERT INTO receivedMessages (chatId, messageHash, messageId, receiverKeyUID, peerId, nodeName, sentAt, topic, messageType, messageSize, createdAt, pubSubTopic, statusVersion, deviceType) VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11, $12, $13, $14) RETURNING id;")
+	tx, err := db.BeginTx(context.Background(), nil)


See prev comment about passing down the context

adklempner · 2024-09-05T22:35:18Z

LGTM. Just left few nitpicks about things that probably werent introduced in this PR and might be worked on later!

Replaced all Prepare with QueryRow (not Exec because Postgres driver doesn't support returning any values this way, and we want the ID) and updated to use passed down context. The latter was really easy to do thanks to the refactoring by @vpavlin !

adklempner marked this pull request as ready for review August 7, 2024 04:23

adklempner mentioned this pull request Aug 7, 2024

feat(telemetry)_: include device type in metrics status-im/status-go#5669

Merged

1 task

igor-sirotin reviewed Aug 7, 2024

View reviewed changes

adklempner force-pushed the feat/device-type branch 3 times, most recently from 0ca0a81 to efe438f Compare August 15, 2024 04:17

adklempner mentioned this pull request Aug 15, 2024

test: throttle peer count metric and add device type status-im/status-desktop#16119

Closed

2 tasks

igor-sirotin reviewed Aug 15, 2024

View reviewed changes

richard-ramos reviewed Aug 15, 2024

View reviewed changes

adklempner force-pushed the feat/device-type branch 2 times, most recently from 193ea15 to cdaa927 Compare August 16, 2024 06:23

igor-sirotin reviewed Aug 16, 2024

View reviewed changes

adklempner changed the title ~~feat: add device type to all metrics~~ feat: refactor tables to reuse common fields Aug 30, 2024

adklempner force-pushed the feat/device-type branch from cdaa927 to 8a5db99 Compare August 30, 2024 17:48

adklempner added 5 commits September 4, 2024 16:34

feat: add device type to all metrics

73137fe

feat: refactor common fields into separate table

a69993f

fix: use tx for common fields and other fixes

1df338e

fix: rename common fields to telemetry record

3f43ebc

fix: remove non-null requirement from telemetry record

9042b0d

adklempner force-pushed the feat/device-type branch from 2c8edfe to d817fcd Compare September 5, 2024 04:26

richard-ramos approved these changes Sep 5, 2024

View reviewed changes

fix: rebase

eba38d4

adklempner force-pushed the feat/device-type branch from d817fcd to eba38d4 Compare September 5, 2024 22:34

adklempner merged commit e14f337 into master Sep 5, 2024
3 checks passed

adklempner deleted the feat/device-type branch September 5, 2024 22:47

	func InsertCommonFields(db *sql.DB, data types.CommonFieldsAccessor) (int, error) {
	func InsertCommonFields(db sql.DB, data types.CommonFields) (int, error) {

	EXECUTE format('ALTER TABLE %I ADD COLUMN IF NOT EXISTS commonFieldsId INTEGER', table_name);
	EXECUTE format('ALTER TABLE %I ADD COLUMN IF NOT EXISTS id INTEGER', table_name);

	_, err := db.Exec(fmt.Sprintf("DROP TABLE IF EXISTS %s", table))
	_, err := db.Exec("DROP TABLE IF EXISTS $1", table)

feat: refactor tables to reuse common fields #36

feat: refactor tables to reuse common fields #36

Conversation

adklempner commented Aug 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

igor-sirotin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

igor-sirotin Aug 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

igor-sirotin commented Aug 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adklempner Aug 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richard-ramos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adklempner commented Sep 5, 2024

adklempner commented Aug 7, 2024 •

edited

Loading

igor-sirotin Aug 15, 2024 •

edited

Loading

adklempner Aug 16, 2024 •

edited

Loading