- 
                Notifications
    You must be signed in to change notification settings 
- Fork 67
ttx service: timeout via context - part 2 #1040
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
| jsonSession := session2.JSON(context) | ||
| msg, err := jsonSession.ReceiveRawWithTimeout(time.Minute * 4) | ||
| span := trace.SpanFromContext(context.Context()) | ||
| span.AddEvent("start_receive_transaction_view") | 
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.
It's better to use the method logger.DebugfContext
| // New returns a JSON-based session wrapping the passed session. | ||
| func New(context context.Context, s Session) *JSession { | ||
| span := trace.SpanFromContext(context) | ||
| span.AddEvent(fmt.Sprintf("Open json session to [%s:%s]", string(s.Info().Caller), s.Info().CallerViewID)) | 
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.
logger
| // New returns a JSON-based session wrapping the passed session. | ||
| func New(context context.Context, s Session) *JSession { | ||
| span := trace.SpanFromContext(context) | ||
| span.AddEvent(fmt.Sprintf("Open json session to [%s:%s]", string(s.Info().Caller), s.Info().CallerViewID)) | 
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.
s.Info() is an expensive call (it contains a lock). I would suggest you use it only once or even better you use the Eval util:
logger.DebugfContext(context, "Open json session: %s", logging.Eval(s.Info))
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.
LGTM modulo comments
08b5077    to
    0528814      
    Compare
  
    clean jsession package FSC b9bece2d0ce57e207809c748f19f431ef0457d62 remove the need to pass a timeout Signed-off-by: Angelo De Caro <[email protected]>
| this will be reworked in another PR | 
This PR requires hyperledger-labs/fabric-smart-client#968.