🏆 Live Soccer TV: EOSIO/eos/pull/

maoueh · 2020-03-09T20:24:01Z

Change Description

Discussed last week, here the dfuse Deep Mind patchset rebased on develop.

It contains low-level dfuse instrumentation of the nodeos binary for post-consumption through a console reader.

Consensus Changes

Consensus Changes

API Changes

API Changes

Documentation Additions

Documentation Additions

maoueh · 2020-03-09T20:24:21Z

I'll rebase all this after the first wave of comments.

abourget · 2020-03-09T20:50:38Z

Things I'm thinking we might want to look:

the action_id, and its alignment (or mis-alignment) with the action ordinals.

arhag

More left to review.

maoueh · 2020-03-11T04:29:02Z

This is ready for a new round of review.

Linked with EOSIO/eos#8788

maoueh · 2020-03-16T07:08:58Z

This is ready for another review round.

Linked with EOSIO/eos#8788

Linked with EOSIO/fc#148

…ature_manager - Those two managers now receives a getter that is able to retrieve a dm_logger instance, if enabled. - Now using a constant for the abi serializer used in deep mind logging statements.

The `generic_ram_trace()` constructor can be used when new RAM calls are being implemented. By using such constructor, it's easy to grep in the source code for all usage of it and properly change the call site to a proper `ram_trace` object.

- Removal of dmlog macro, now using `fc_dlog` directly - Put back deep mind logger in `initialize_logging` - Unbuffered stdout is now performed only when initializing deep mind

maoueh · 2020-03-17T14:41:34Z

Pushed with latest changes @arhag @heifner

maoueh · 2020-03-19T15:40:47Z

Updated to now merged deep mind branch on fc.

…lated code

maoueh · 2020-03-20T18:58:08Z

@heifner @arhag New comments addressed.

maoueh · 2020-03-25T03:05:47Z

Anything left to be done/changed here. Would like to get that out in the wild.

arhag

@maoueh:

Please don't force push to the branch. It prevents doing incremental reviews to check if the requested changes were implemented correctly and forces a re-review of the whole PR at each iteration to ensure nothing was missed.

And along those same lines, please allow the reviewer to resolve the conversations they started (feel free to reply "Done" or something similar though) because it makes it easier for the reviewer to keep track of which of their requested changes they had already confirmed were properly addressed.

maoueh · 2020-03-27T01:23:46Z

@arhag Yeah indeed, I'm used to rebase my own branches, never realized it breaks this review flow. Will follow that as well as the Resolve Conversation comment. I'm sorry to give you more work.

Those are good comments.

arhag · 2020-03-27T03:21:01Z

@maoueh: No worries. Also, I forgot to mention: make sure to bring in the latest changes from the develop branch and resolve the existing merge conflicts.

…mind.{hpp,cpp} files and more

maoueh · 2020-03-27T06:01:57Z

@arhag Updated PR against your latest comments.

arhag

I also left a suggestion regarding ram_trace here: #8788 (comment).

And don't forget to address the option description as discussed here: #8788 (comment)

- ram_trace change - fixed deep-mind option description - removed ABI serializer for block - moved EOSIO_DEVELOPER to right location(s)

- Added `maybe_to_variant_with_abi` so that ABI serialization for deep mind purposes is never failing, will now log as a warning the error exception and return the object untransformed. - Fix some comments in the code.

We now use `abi_serializer_max_time_us` directly instead.

nksanthosh requested a review from arhag March 9, 2020 21:20

gleehokie reviewed Mar 9, 2020

View reviewed changes

Comment thread libraries/chain/apply_context.cpp Resolved

gleehokie reviewed Mar 9, 2020

View reviewed changes

Comment thread libraries/chain/apply_context.cpp Resolved

gleehokie reviewed Mar 9, 2020

View reviewed changes

Comment thread libraries/chain/apply_context.cpp Resolved

arhag suggested changes Mar 10, 2020

View reviewed changes

maoueh force-pushed the feature/deep-mind branch from 255f179 to 40c8bb5 Compare March 11, 2020 03:17

maoueh commented Mar 11, 2020

View reviewed changes

Comment thread programs/nodeos/main.cpp Resolved

heifner suggested changes Mar 11, 2020

View reviewed changes

Comment thread libraries/dm/include/dfuse/dm/log_appender.hpp Resolved

Comment thread libraries/dm/log_appender.cpp Resolved

Comment thread programs/nodeos/main.cpp Resolved

Comment thread programs/nodeos/main.cpp Resolved

arhag suggested changes Mar 11, 2020

View reviewed changes

Comment thread libraries/chain/include/eosio/chain/protocol_feature_manager.hpp Resolved

Comment thread libraries/chain/include/eosio/chain/resource_limits.hpp Resolved

Comment thread unittests/resource_limits_test.cpp Resolved

Comment thread libraries/chain/apply_context.cpp Resolved

maoueh pushed a commit to dfuse-io/fc that referenced this pull request Mar 13, 2020

Extract of deep mind logger (and macro) in libraries/fc directly

742854b

Linked with EOSIO/eos#8788

maoueh mentioned this pull request Mar 13, 2020

Extract of deep mind logger in libraries/fc directly EOSIO/fc#148

Merged

maoueh pushed a commit to dfuse-io/fc that referenced this pull request Mar 16, 2020

Extract of deep mind logger (and macro) in libraries/fc directly

77557d0

Linked with EOSIO/eos#8788

maoueh force-pushed the feature/deep-mind branch from b064e8d to 2c44f80 Compare March 16, 2020 02:12

maoueh pushed a commit to dfuse-io/fc that referenced this pull request Mar 17, 2020

Extract of deep mind logger (and macro) in libraries/fc directly

ad07254

Linked with EOSIO/eos#8788

Matthieu Vachon and others added 8 commits March 17, 2020 10:38

Initial commit for dfuse Deep Mind changes

55826d8

Fix typo

1ecb3bd

First batch of changes for first review

8a2ae50

Fixed logger name and re-work dmlog macro to be as lean as possible

fb6b1de

Moved deep mind logging code to libraries/fc directly

7fbf213

Linked with EOSIO/fc#148

Remove controller dependency on resource_limits_manager & protocol_fe…

9b80bcb

…ature_manager - Those two managers now receives a getter that is able to retrieve a dm_logger instance, if enabled. - Now using a constant for the abi serializer used in deep mind logging statements.

More changes from review round

f72589f

- Removal of dmlog macro, now using `fc_dlog` directly - Put back deep mind logger in `initialize_logging` - Unbuffered stdout is now performed only when initializing deep mind

maoueh force-pushed the feature/deep-mind branch from d146271 to f72589f Compare March 17, 2020 14:39

Matthieu Vachon added 2 commits March 17, 2020 11:53

Fix compilation errors, use yield pattern

16ce65f

Updated libraries/fc to latest

9877b08

Bumped fc to merged deep mind branch

ee94eaa

heifner suggested changes Mar 19, 2020

View reviewed changes

Comment thread libraries/chain/apply_context.cpp Resolved

Comment thread libraries/chain/controller.cpp Resolved

Comment thread libraries/chain/include/eosio/chain/types.hpp Resolved

Comment thread programs/nodeos/main.cpp Resolved

Added deep_mind.hpp,cpp files to contain some of the deep-mind re…

37cb7b3

…lated code

maoueh commented Mar 26, 2020

View reviewed changes

Comment thread libraries/chain/CMakeLists.txt Resolved

maoueh commented Mar 26, 2020

View reviewed changes

Comment thread unittests/resource_limits_test.cpp Resolved

maoueh commented Mar 26, 2020

View reviewed changes

Comment thread libraries/chain/controller.cpp Resolved

arhag suggested changes Mar 27, 2020

View reviewed changes

Matthieu Vachon added 2 commits March 27, 2020 01:40

Adressed review comments regarding ram_trace, useless includes, deep_…

2cf00aa

…mind.{hpp,cpp} files and more

Merge branch 'develop' into feature/deep-mind

791a42b

maoueh commented Mar 27, 2020

View reviewed changes

Comment thread plugins/chain_plugin/chain_plugin.cpp Resolved

arhag suggested changes Mar 27, 2020

View reviewed changes

Comment thread libraries/chain/CMakeLists.txt Resolved

Matthieu Vachon added 2 commits March 27, 2020 18:16

More changes for review

810e3ea

- ram_trace change - fixed deep-mind option description - removed ABI serializer for block - moved EOSIO_DEVELOPER to right location(s)

Removed left over commented code

fc627a3

heifner suggested changes Apr 6, 2020

View reviewed changes

Matthieu Vachon added 3 commits April 8, 2020 22:36

Updated deep mind patch with latest review comments

40c9c5d

- Added `maybe_to_variant_with_abi` so that ABI serialization for deep mind purposes is never failing, will now log as a warning the error exception and return the object untransformed. - Fix some comments in the code.

Merge branch 'develop' into feature/deep-mind

4856a00

Removed dmlog_abi_serializer_max_time_us

ca370ba

We now use `abi_serializer_max_time_us` directly instead.

heifner approved these changes Apr 9, 2020

View reviewed changes

arhag approved these changes Apr 10, 2020

View reviewed changes

arhag merged commit c271dd0 into EOSIO:develop Apr 10, 2020

maoueh deleted the feature/deep-mind branch April 10, 2020 15:10

Conversation

maoueh commented Mar 9, 2020

Change Description

Consensus Changes

API Changes

Documentation Additions

Uh oh!

maoueh commented Mar 9, 2020

Uh oh!

abourget commented Mar 9, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

arhag left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

maoueh commented Mar 11, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

maoueh commented Mar 16, 2020

Uh oh!

maoueh commented Mar 17, 2020

Uh oh!

maoueh commented Mar 19, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

maoueh commented Mar 20, 2020

Uh oh!

maoueh commented Mar 25, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

arhag left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

maoueh commented Mar 27, 2020

Uh oh!

arhag commented Mar 27, 2020

Uh oh!

maoueh commented Mar 27, 2020

Uh oh!

Uh oh!

arhag left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!