-
Notifications
You must be signed in to change notification settings - Fork 242
Blockchain metrics for Ethereum #584
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.
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
Conversation
Signed-off-by: Nicko Guyer <nicko.guyer@kaleido.io>
Signed-off-by: Nicko Guyer <nicko.guyer@kaleido.io>
Codecov Report
@@ Coverage Diff @@
## main #584 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 309 310 +1
Lines 18705 18763 +58
=========================================
+ Hits 18705 18763 +58
Continue to review full report at Codecov.
|
Signed-off-by: Nicko Guyer <nicko.guyer@kaleido.io>
pkg/fftypes/blockchainevent.go
Outdated
| Info JSONObject `json:"info,omitempty"` | ||
| Timestamp *FFTime `json:"timestamp,omitempty"` | ||
| TX TransactionRef `json:"tx"` | ||
| Location string `json:"-"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to avoid these?
e.g. only include them on blockchain.Event, rather than fftypes.BlockchainEvent which is an external API object.
If we keep them here, they definitely need comments to explain they aren't available.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Possibly. Let me try that.
Signed-off-by: Nicko Guyer <nicko.guyer@kaleido.io>
Signed-off-by: Nicko Guyer <nicko.guyer@kaleido.io>
1a8bf1d to
b4fde14
Compare
Signed-off-by: Nicko Guyer <nicko.guyer@kaleido.io>
| if err := em.persistBlockchainEvent(ctx, chainEvent); err != nil { | ||
| return err | ||
| } | ||
| em.emitBlockchainEventMetric(*ev) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just wondering why the mem copy here, rather than a pass-by-reference?
I think the blockchain.Event structure is reasonably large.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch. I've adjusted this, thanks!
peterbroadhurst
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes look great thanks @nguyer
Marked approval with one minor efficiency comment to consider (passing &event.Event rather than event.Event)
Signed-off-by: Nicko Guyer <nicko.guyer@kaleido.io>
This PR introduces blockchain metrics for:
Each of these has two labels
locationandmethodNamefor transactions/queries orsignature(for events). Thelocationis a blockchain specific appropriate string for describing where the event came from. The value of this label isin the format of
"<key1>=<value1>,<key2>=<value2>". For Ethereum, it is justaddress=<contract_address>.Fabric metrics will also be added in a subsequent PR.
Resolves #559