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.

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

Add structured logging for TraceSendRecv TxSubmission objects #584

Merged
merged 2 commits into from
Feb 19, 2020

Conversation

intricate
Copy link
Contributor

@intricate intricate commented Feb 18, 2020

No description provided.

@intricate intricate self-assigned this Feb 18, 2020
Comment on lines +868 to +879
toObject _verb (MsgRequestTxIds _ _ _) =
mkObject
[ "kind" .= String "MsgRequestTxIds"
]
toObject _verb (MsgReplyTxIds _) =
mkObject
[ "kind" .= String "MsgReplyTxIds"
]
toObject _verb MsgDone =
mkObject
[ "kind" .= String "MsgDone"
]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this going to be too noisy being logged at info?

Copy link
Contributor

Choose a reason for hiding this comment

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

if these messages are useful to see in production then "Info" is right.

Copy link
Contributor Author

@intricate intricate Feb 18, 2020

Choose a reason for hiding this comment

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

Just a note: since MsgRequestTxs and MsgReplyTxs are the only useful constructors to log right now, I added a transformer in Cardano.Tracing.Tracers (see traceMsgRequestAndReplyTxs) to ensure that those are the only ones that are traced.

@intricate intricate marked this pull request as ready for review February 18, 2020 14:59
@CodiePP CodiePP added this to the S7 2020-02-27 milestone Feb 18, 2020
@CodiePP CodiePP added the byron Required for a Byron mainnet: replace the old core nodes with cardano-node. label Feb 18, 2020
@@ -195,7 +195,7 @@ mkTracers traceOptions tracer = do
, consensusTracers
= mkConsensusTracers forgeTracers traceOptions
, protocolTracers
= mkProtocolsTracers traceOptions
= mkProtocolTracers traceOptions
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

Copy link
Contributor

@CodiePP CodiePP left a comment

Choose a reason for hiding this comment

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

👍

Comment on lines +868 to +879
toObject _verb (MsgRequestTxIds _ _ _) =
mkObject
[ "kind" .= String "MsgRequestTxIds"
]
toObject _verb (MsgReplyTxIds _) =
mkObject
[ "kind" .= String "MsgReplyTxIds"
]
toObject _verb MsgDone =
mkObject
[ "kind" .= String "MsgDone"
]
Copy link
Contributor

Choose a reason for hiding this comment

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

if these messages are useful to see in production then "Info" is right.

@intricate intricate force-pushed the intricate/527-2 branch 4 times, most recently from 0aef763 to a0ec6de Compare February 18, 2020 19:58
Copy link
Contributor

@Jimbo4350 Jimbo4350 left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Contributor

@dcoutts dcoutts left a comment

Choose a reason for hiding this comment

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

I'm a little nervous about putting this into the low level protocol tracer. It is quite a high volume tracer. I'd be happier if we could do this in the tracer for the higher level "Inbound" and "Outbound" client & server implementations. This lets us be more precise about logging just the things we need, rather than every single message (and having to filter most of it out).

We already have a TraceTxSubmissionInbound and a TraceTxSubmissionOutbound.

So for our requirement:

We also need to trace at the default log level for the outbound side of tx submission:

  • when a remote peer has requested a tx id, and we do send the tx to them, then we should log that the txid was requested and sent

I think we can do it just by extending the TraceTxSubmissionOutbound with the right thing and using it in the right place, and tracing that at default log level.

@intricate
Copy link
Contributor Author

Although this PR is no longer related to #527, it still adds structured logging for TxSubmission at the protocol tracer level. So I've instead changed this back to logging at debug severity.

@intricate
Copy link
Contributor Author

bors r+

iohk-bors bot added a commit that referenced this pull request Feb 19, 2020
584: Add structured logging for TraceSendRecv TxSubmission objects r=intricate a=intricate



Co-authored-by: Luke Nadur <[email protected]>
@iohk-bors
Copy link
Contributor

iohk-bors bot commented Feb 19, 2020

@iohk-bors iohk-bors bot merged commit 1e029a0 into master Feb 19, 2020
@iohk-bors iohk-bors bot deleted the intricate/527-2 branch February 19, 2020 15:53
iohk-bors bot added a commit to IntersectMBO/ouroboros-network that referenced this pull request Feb 26, 2020
1688: Trace transaction requests and replies in txSubmissionOutbound r=intricate a=intricate

Related to IntersectMBO/cardano-node#527.

More specifically, see IntersectMBO/cardano-node#584 (review).

Co-authored-by: Luke Nadur <[email protected]>
iohk-bors bot added a commit that referenced this pull request Mar 3, 2020
624: Implement ToObject instance for TraceTxSubmissionOutbound r=intricate a=intricate

Closes #527

See #584 (review)

Co-authored-by: Luke Nadur <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
byron Required for a Byron mainnet: replace the old core nodes with cardano-node.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants