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

fix(dot/telemetry): telemetry hashes to be in the hexadecimal format #2194

Merged
merged 6 commits into from
Jan 17, 2022
Merged

fix(dot/telemetry): telemetry hashes to be in the hexadecimal format #2194

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
79c8b95
fix: telemetry hashes must to be in hexadecimal format
EclesioMeloJunior Jan 14, 2022
2dd9160
chore: fix `lll` lint warn
EclesioMeloJunior Jan 14, 2022
d9addfd
chore: use strict type in `common.Hash.MarshaJSON()`
EclesioMeloJunior Jan 14, 2022
a73a4dd
chore: use right defined type
EclesioMeloJunior Jan 14, 2022
2b98e9f
chore: using strict type
EclesioMeloJunior Jan 14, 2022
9056f43
chore: remove complex anon functions at telemetry `SendMulti` test
EclesioMeloJunior Jan 17, 2022
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
1 change: 0 additions & 1 deletion dot/telemetry/afg_authority_set.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,6 @@ func NewAfgAuthoritySet(authorityID, authoritySetID, authorities string) *AfgAut
}
}

// MarshalJSON ...
func (afg AfgAuthoritySet) MarshalJSON() ([]byte, error) {
telemetryData := struct {
afgAuthoritySetTM
Expand Down
68 changes: 37 additions & 31 deletions dot/telemetry/mailer_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,14 +63,14 @@ func TestHandler_SendMulti(t *testing.T) {
[]byte(`{"best":"0x07b749b6e20fd5f1159153a2e790235018621dd06072a62bcd25e8576f6ff5e6","height":2,"origin":"NetworkInitialSync","msg":"block.import","ts":`), //nolint:lll
[]byte(`{"bandwidth_download":2,"bandwidth_upload":3,"peers":1,"msg":"system.interval","ts":`),
[]byte(`{"best":"0x07b749b6e20fd5f1159153a2e790235018621dd06072a62bcd25e8576f6ff5e6","height":32375,"finalized_hash":"0x687197c11b4cf95374159843e7f46fbcd63558db981aaef01a8bac2a44a1d6b2","finalized_height":32256,"txcount":0,"used_state_cache_size":1234,"msg":"system.interval","ts":`), //nolint:lll
[]byte(`{"best":[7,183,73,182,226,15,213,241,21,145,83,162,231,144,35,80,24,98,29,208,96,114,166,43,205,37,232,87,111,111,245,230],"height":"32375","msg":"notify.finalized","ts":`), //nolint:lll
[]byte(`{"hash":[88,20,174,195,226,133,39,248,31,101,132,30,3,72,114,243,163,3,55,207,108,51,178,210,88,187,166,7,30,55,226,124],"number":"1","msg":"prepared_block_for_proposing","ts":`), //nolint:lll
[]byte(`{"best":"0x07b749b6e20fd5f1159153a2e790235018621dd06072a62bcd25e8576f6ff5e6","height":"32375","msg":"notify.finalized","ts":`), //nolint:lll
[]byte(`{"hash":"0x5814aec3e28527f81f65841e034872f3a30337cf6c33b2d258bba6071e37e27c","number":"1","msg":"prepared_block_for_proposing","ts":`), //nolint:lll
[]byte(`{"ready":1,"future":2,"msg":"txpool.import","ts":`),
[]byte(`{"authority_id":"authority_id","authority_set_id":"authority_set_id","authorities":"json-stringified-ids-of-authorities","msg":"afg.authority_set","ts`), //nolint:lll
[]byte(`{"hash":[7,183,73,182,226,15,213,241,21,145,83,162,231,144,35,80,24,98,29,208,96,114,166,43,205,37,232,87,111,111,245,230],"number":"1","msg":"afg.finalized_blocks_up_to","ts":`), //nolint:lll
[]byte(`{"target_hash":[88,20,174,195,226,133,39,248,31,101,132,30,3,72,114,243,163,3,55,207,108,51,178,210,88,187,166,7,30,55,226,124],"target_number":"1","contains_precommits_signed_by":[],"msg":"afg.received_commit","ts":`), //nolint:lll
[]byte(`{"target_hash":[88,20,174,195,226,133,39,248,31,101,132,30,3,72,114,243,163,3,55,207,108,51,178,210,88,187,166,7,30,55,226,124],"target_number":"1","voter":"","msg":"afg.received_precommit","ts":`), //nolint:lll
[]byte(`{"target_hash":[88,20,174,195,226,133,39,248,31,101,132,30,3,72,114,243,163,3,55,207,108,51,178,210,88,187,166,7,30,55,226,124],"target_number":"1","voter":"","msg":"afg.received_prevote","ts":`), //nolint:lll
[]byte(`{"authority_id":"authority_id","authority_set_id":"authority_set_id","authorities":"json-stringified-ids-of-authorities","msg":"afg.authority_set","ts`), //nolint:lll
[]byte(`{"hash":"0x07b749b6e20fd5f1159153a2e790235018621dd06072a62bcd25e8576f6ff5e6","number":"1","msg":"afg.finalized_blocks_up_to","ts":`), //nolint:lll
[]byte(`{"target_hash":"0x5814aec3e28527f81f65841e034872f3a30337cf6c33b2d258bba6071e37e27c","target_number":"1","contains_precommits_signed_by":[],"msg":"afg.received_commit","ts":`), //nolint:lll
[]byte(`{"target_hash":"0x5814aec3e28527f81f65841e034872f3a30337cf6c33b2d258bba6071e37e27c","target_number":"1","voter":"","msg":"afg.received_precommit","ts":`), //nolint:lll
[]byte(`{"target_hash":"0x5814aec3e28527f81f65841e034872f3a30337cf6c33b2d258bba6071e37e27c","target_number":"1","voter":"","msg":"afg.received_prevote","ts":`), //nolint:lll
}

messages := []Message{
Expand All @@ -95,23 +95,34 @@ func TestHandler_SendMulti(t *testing.T) {
),

NewAfgAuthoritySet("authority_id", "authority_set_id", "json-stringified-ids-of-authorities"),
NewAfgFinalizedBlocksUpTo(
common.MustHexToHash("0x07b749b6e20fd5f1159153a2e790235018621dd06072a62bcd25e8576f6ff5e6"), "1"),
NewAfgReceivedCommit(
common.MustHexToHash("0x5814aec3e28527f81f65841e034872f3a30337cf6c33b2d258bba6071e37e27c"),

func(hash common.Hash, number string) Message {
return NewAfgFinalizedBlocksUpTo(hash, number)
}(common.MustHexToHash("0x07b749b6e20fd5f1159153a2e790235018621dd06072a62bcd25e8576f6ff5e6"), "1"),
Copy link
Contributor

Choose a reason for hiding this comment

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

Cant't we define an implementation of Message directly instead of this convoluted wrapping?


func(targetHash common.Hash, targetNumber string, containsPrecommitsSignedBy []string) Message {
return NewAfgReceivedCommit(targetHash, targetNumber, containsPrecommitsSignedBy)
}(common.MustHexToHash("0x5814aec3e28527f81f65841e034872f3a30337cf6c33b2d258bba6071e37e27c"),
"1", []string{}),
NewAfgReceivedPrecommit(
common.MustHexToHash("0x5814aec3e28527f81f65841e034872f3a30337cf6c33b2d258bba6071e37e27c"),

func(targetHash common.Hash, targetNumber string, voter string) Message {
return NewAfgReceivedPrecommit(targetHash, targetNumber, voter)
}(common.MustHexToHash("0x5814aec3e28527f81f65841e034872f3a30337cf6c33b2d258bba6071e37e27c"),
"1", ""),
NewAfgReceivedPrevote(
common.MustHexToHash("0x5814aec3e28527f81f65841e034872f3a30337cf6c33b2d258bba6071e37e27c"),

func(targetHash common.Hash, targetNumber string, voter string) Message {
return NewAfgReceivedPrevote(targetHash, targetNumber, voter)
}(common.MustHexToHash("0x5814aec3e28527f81f65841e034872f3a30337cf6c33b2d258bba6071e37e27c"),
"1", ""),

NewNotifyFinalized(
common.MustHexToHash("0x07b749b6e20fd5f1159153a2e790235018621dd06072a62bcd25e8576f6ff5e6"),
func(best common.Hash, height string) Message {
return NewNotifyFinalized(best, height)
}(common.MustHexToHash("0x07b749b6e20fd5f1159153a2e790235018621dd06072a62bcd25e8576f6ff5e6"),
"32375"),
NewPreparedBlockForProposing(
common.MustHexToHash("0x5814aec3e28527f81f65841e034872f3a30337cf6c33b2d258bba6071e37e27c"),

func(hash common.Hash, number string) Message {
return NewPreparedBlockForProposing(hash, number)
}(common.MustHexToHash("0x5814aec3e28527f81f65841e034872f3a30337cf6c33b2d258bba6071e37e27c"),
"1"),
}

Expand Down Expand Up @@ -262,8 +273,7 @@ func TestTelemetryMarshalMessage(t *testing.T) {
Hash: common.Hash{},
Number: "0",
},
expected: regexp.MustCompile(`^{"hash":\[0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,` +
`0,0,0,0,0,0,0,0,0,0,0,0,0,0,0\],"number":"0",` +
expected: regexp.MustCompile(`^{"hash":"0x[0]{64}","number":"0",` +
`"msg":"afg.finalized_blocks_up_to","ts":"[0-9]{4}-[0-9]{2}-[0-9]{2}T[0-9]{2}:[0-9]{2}:` +
`[0-9]{2}.[0-9]+Z|([+-][0-9]{2}:[0-9]{2})"}$`),
},
Expand All @@ -273,8 +283,7 @@ func TestTelemetryMarshalMessage(t *testing.T) {
TargetNumber: "0",
Voter: "0x0",
},
expected: regexp.MustCompile(`^{"target_hash":\[0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,` +
`0,0,0,0,0,0,0,0,0,0,0,0,0,0,0\],"target_number":"0","voter":"0x0",` +
expected: regexp.MustCompile(`^{"target_hash":"0x[0]{64}","target_number":"0","voter":"0x0",` +
`"msg":"afg.received_precommit","ts":"[0-9]{4}-[0-9]{2}-[0-9]{2}T[0-9]{2}:[0-9]{2}:` +
`[0-9]{2}.[0-9]+Z|([+-][0-9]{2}:[0-9]{2})"}$`),
},
Expand All @@ -284,8 +293,7 @@ func TestTelemetryMarshalMessage(t *testing.T) {
TargetNumber: "0",
Voter: "0x0",
},
expected: regexp.MustCompile(`^{"target_hash":\[0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,` +
`0,0,0,0,0,0,0,0,0,0,0,0,0,0,0\],"target_number":"0","voter":"0x0",` +
expected: regexp.MustCompile(`^{"target_hash":"0x[0]{64}","target_number":"0","voter":"0x0",` +
`"msg":"afg.received_prevote","ts":"[0-9]{4}-[0-9]{2}-[0-9]{2}T[0-9]{2}:[0-9]{2}:` +
`[0-9]{2}.[0-9]+Z|([+-][0-9]{2}:[0-9]{2})"}$`),
},
Expand All @@ -295,8 +303,8 @@ func TestTelemetryMarshalMessage(t *testing.T) {
TargetNumber: "0",
ContainsPrecommitsSignedBy: []string{"0x0", "0x1"},
},
expected: regexp.MustCompile(`^{"target_hash":\[0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,` +
`0,0,0,0,0,0,0,0,0,0,0,0,0,0,0\],"target_number":"0","contains_precommits_signed_by":\["0x0","0x1"\],` +
expected: regexp.MustCompile(`^{"target_hash":"0x[0]{64}","target_number":"0",` +
`"contains_precommits_signed_by":\["0x0","0x1"\],` +
`"msg":"afg.received_commit","ts":"[0-9]{4}-[0-9]{2}-[0-9]{2}T[0-9]{2}:[0-9]{2}:` +
`[0-9]{2}.[0-9]+Z|([+-][0-9]{2}:[0-9]{2})"}$`),
},
Expand All @@ -315,8 +323,7 @@ func TestTelemetryMarshalMessage(t *testing.T) {
Best: common.Hash{},
Height: "0",
},
expected: regexp.MustCompile(`^{"best":\[0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,` +
`0,0,0,0,0,0,0,0,0,0,0,0,0,0,0\],"height":"0",` +
expected: regexp.MustCompile(`^{"best":"0x[0]{64}","height":"0",` +
`"msg":"notify.finalized","ts":"[0-9]{4}-[0-9]{2}-[0-9]{2}T[0-9]{2}:[0-9]{2}:` +
`[0-9]{2}.[0-9]+Z|([+-][0-9]{2}:[0-9]{2})"}$`),
},
Expand All @@ -325,8 +332,7 @@ func TestTelemetryMarshalMessage(t *testing.T) {
Hash: common.Hash{},
Number: "0",
},
expected: regexp.MustCompile(`^{"hash":\[0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,` +
`0,0,0,0,0,0,0,0,0,0,0,0,0,0,0\],"number":"0",` +
expected: regexp.MustCompile(`^{"hash":"0x[0]{64}","number":"0",` +
`"msg":"prepared_block_for_proposing","ts":"[0-9]{4}-[0-9]{2}-[0-9]{2}T[0-9]{2}:[0-9]{2}:` +
`[0-9]{2}.[0-9]+Z|([+-][0-9]{2}:[0-9]{2})"}$`),
},
Expand Down
2 changes: 1 addition & 1 deletion lib/common/hash.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,7 @@ func (h *Hash) UnmarshalJSON(data []byte) error {
}

// MarshalJSON converts hash to hex data
func (h *Hash) MarshalJSON() ([]byte, error) {
func (h Hash) MarshalJSON() ([]byte, error) {
Copy link
Member Author

Choose a reason for hiding this comment

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

this is needed since there are telemetry message fields that uses common.Hash and fields that uses *common.Hash and when the message is marshaled the output to common.Hash is [...] and the output to *common.Hash is 0x... and now the output for both uses is 0x...

Copy link
Contributor

@qdm12 qdm12 Jan 17, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Nice catch!!! Alternatively should we use named fields everywhere instead of 'inheriting'? To be fair inheritance is just scary in Go (except for composing interfaces) Monday morning, sorry!

return json.Marshal(h.String())
}

Expand Down