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

CAD-3511: datapoint-forward library. #3303

Closed
wants to merge 1 commit into from

Conversation

denisshevchenko
Copy link
Contributor

@denisshevchenko denisshevchenko commented Oct 20, 2021

The third forwarding library for the new tracing infrastructure (#2847). It allows forwarding arbitrary data that provides ToJSON instance. The trace-dispatcher will use this library to forward different structured data to external acceptor apps (for example, cardano-tracer).

For reviewers: please note that this library is based on the existing trace-forward library, but with one important difference: there is no "infinite stream" of data when we ask for it every N seconds. Instead, acceptor app asks for each DataPoint explicitly, when it needs this data.

@denisshevchenko
Copy link
Contributor Author

denisshevchenko commented Nov 1, 2021

Please note that after it will be merged, NodeInfo will be removed from trace-forward library (https://github.com/input-output-hk/cardano-node/blob/master/trace-forward/src/Trace/Forward/Protocol/Type.hs#L125-L132), because it will be provided by the node itself and will be forwarded as one of DataPoints.

@denisshevchenko denisshevchenko force-pushed the cad-3511-datapoint-forward-library branch from 6ae8262 to 9fb279f Compare November 1, 2021 13:09
iohk-bors bot added a commit that referenced this pull request Nov 2, 2021
3337: CAD-3628: remove NodeInfo from trace-forward. r=deepfire a=denisshevchenko

Since the new `datapoint-forward` library (#3303) will be used to forward structured data from the node, `NodeInfo` type should be removed from `trace-forward` library.

Co-authored-by: Denis Shevchenko <denis.shevchenko@iohk.io>
iohk-bors bot added a commit that referenced this pull request Nov 2, 2021
3337: CAD-3628: remove NodeInfo from trace-forward. r=denisshevchenko a=denisshevchenko

Since the new `datapoint-forward` library (#3303) will be used to forward structured data from the node, `NodeInfo` type should be removed from `trace-forward` library.

Co-authored-by: Denis Shevchenko <denis.shevchenko@iohk.io>
iohk-bors bot added a commit that referenced this pull request Nov 3, 2021
3337: CAD-3628: remove NodeInfo from trace-forward. r=denisshevchenko a=denisshevchenko

Since the new `datapoint-forward` library (#3303) will be used to forward structured data from the node, `NodeInfo` type should be removed from `trace-forward` library.

Co-authored-by: Denis Shevchenko <denis.shevchenko@iohk.io>
Copy link
Contributor

@coot coot left a comment

Choose a reason for hiding this comment

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

Could you provide a high level description how does it fit into node and the trace-forwarder library.

I provided various feedback, the most important is to not use UnversionedProtocol, but provided it's own versioning scheme which would distniguish it from all other protocols (trace-forwarder, node-to-client or even node-to-node).


tests :: TestTree
tests = testGroup "DataPoint.Forward.Protocol"
[ testProperty "codec" prop_codec_DataPointForward
Copy link
Contributor

Choose a reason for hiding this comment

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

You should expand the test in this module. Please check keep-alive test suite as a template.

datapoint-forward/datapoint-forward.cabal Show resolved Hide resolved
datapoint-forward/datapoint-forward.cabal Show resolved Hide resolved
@denisshevchenko
Copy link
Contributor Author

Closed because of #3342

@denisshevchenko denisshevchenko deleted the cad-3511-datapoint-forward-library branch November 4, 2021 08:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants