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-3243: trace-forward library. #2960

Merged
merged 1 commit into from
Aug 12, 2021
Merged

CAD-3243: trace-forward library. #2960

merged 1 commit into from
Aug 12, 2021

Conversation

denisshevchenko
Copy link
Contributor

@denisshevchenko denisshevchenko commented Jul 25, 2021

This is trace-forward library: typed-protocols-based library allowing to forward tracing items from one process to another one. This is the first step toward a new tracing infrastructure (please see #2847). The trace-dispatcher will use this library to forward TraceObjects to external acceptors (for example, cardano-tracer).

You can think of this library as a replacement for old plugins trace-forwarder and trace-acceptor from iohk-monitoring-framework.

Copy link
Contributor

@deepfire deepfire left a comment

Choose a reason for hiding this comment

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

LGTM!

It's great to finally have things moving towards the final merge!

@denisshevchenko
Copy link
Contributor Author

@newhoggy Thanks! Fixed.

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.

One minor addition and then I'm happy to approve

trace-forward/README.md Show resolved Hide resolved
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.

LGTM, @denisshevchenko thanks for cooperation!

@denisshevchenko
Copy link
Contributor Author

bors merge

iohk-bors bot added a commit that referenced this pull request Aug 10, 2021
2960: CAD-3243: trace-forward library. r=denisshevchenko a=denisshevchenko

This is `trace-forward` library: `typed-protocols`-based library allowing to forward tracing items from one process to another one. This is the first step toward a new tracing infrastructure (please see #2847). The `trace-dispatcher` will use this library to forward `TraceObject`s to external acceptors (for example, `cardano-tracer`).

You can think of this library as a **_replacement_** for old plugins [trace-forwarder](https://github.com/input-output-hk/iohk-monitoring-framework/tree/master/plugins/backend-trace-forwarder) and [trace-acceptor](https://github.com/input-output-hk/iohk-monitoring-framework/tree/master/plugins/backend-trace-acceptor) from `iohk-monitoring-framework`.

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

bors r-

@iohk-bors
Copy link
Contributor

iohk-bors bot commented Aug 10, 2021

Canceled.

@deepfire
Copy link
Contributor

Marcin has one last question.

@denisshevchenko denisshevchenko force-pushed the cad-3243-trace-forward branch 2 times, most recently from 2707d9a to 0c97ec5 Compare August 10, 2021 19:30
Comment on lines 163 to 164
runActionWithTimer (niHandler replyWithNI) limitInSecs
readIORef shouldWeStop <&> acceptorActions config loHandler niHandler False
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not do what I asked for. I wanted the whole acceptActions to terminate if the IORef is set to true and does not terminate within a timeout, now it will timeout the niHandler action but it has nothing to do with the IORef.

Copy link
Contributor

@coot coot Aug 11, 2021

Choose a reason for hiding this comment

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

I'd suggest to turn the IORef into a TVar and use it in runActionWithTimer. It will require two stm transactions first would block on on this tvar. Then one can create a timerVar and block on it, something like this:

timeoutWhenStopped :: TVar Bool -> Int -> IO a -> IO (Maybe a)
timeoutWhenStoppepd stopVar delay io =
  fmap (either Just (\_ -> Nothing)) $
  race io
     $ do atomically (readTVar stopVar >>= check)
          v <- registerDealay delay    
          atomically (readTVar v >>= check)       

You cannot apply it to TraceAcceptor type directly, since it's not an IO action, but you can plug it where you run the acceptor using runPeer.

Copy link
Contributor

Choose a reason for hiding this comment

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

@coot, what you are alluding to is quite involved -- can you suggest a concrete change -- we'll be happy to accept.

Copy link
Contributor

Choose a reason for hiding this comment

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

The biggest change is IORef -> TVar, but it's a mechanical change - so quite easy to do. Other than that it's just wrapping runPeer inside timeoutWhenStopped.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

wrapping runPeer inside timeoutWhenStopped

Honestly, I cannot understand why?? I do understand why we need stop timer - to avoid too long (theoretically - infinite) time for performing of external handlers. And now it's STM-transactions with orElse - ok, I understand. But why do we need to wrap runPeer inside timeoutWhenStopped? runPeer calls acceptorActions, acceptorActions calls runActionWithTimer...

Copy link
Contributor

@coot coot Aug 11, 2021

Choose a reason for hiding this comment

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

Peer, which is a result of calling acceptorActions is just a data type, you cannot effectively limit how long it will execute after the stop var was set to True, or do any other resource handling. You can only do that around runPeer which returns an IO action, IO actions can be timed out.

Copy link
Contributor Author

@denisshevchenko denisshevchenko Aug 11, 2021

Choose a reason for hiding this comment

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

Ok. But I still don't understand how to do it exactly. Are there any existing typed-protocols in ouroboros-network where I can find an example of it? And does any from existing protocols have such a TVar-based stopping mechanism?

Yes, my level isn't very high: I'm not deeply familiar with STM, I don't deeply understand typed-protocol inside and did a lot of things just by example. Sorry, it's true. And I took responsibility for trace-forward not because I'm the best person for it, but because we don't have another person for it... That's why existing example would be the best option for me.

@denisshevchenko denisshevchenko force-pushed the cad-3243-trace-forward branch 4 times, most recently from 79e60f6 to c6f8f75 Compare August 11, 2021 10:30
@IntersectMBO IntersectMBO deleted a comment from denisshevchenko Aug 11, 2021
@IntersectMBO IntersectMBO deleted a comment from denisshevchenko Aug 11, 2021
@IntersectMBO IntersectMBO deleted a comment from denisshevchenko Aug 11, 2021
@denisshevchenko
Copy link
Contributor Author

bors r+

@iohk-bors
Copy link
Contributor

iohk-bors bot commented Aug 12, 2021

@iohk-bors iohk-bors bot merged commit bf71fe3 into master Aug 12, 2021
@iohk-bors iohk-bors bot deleted the cad-3243-trace-forward branch August 12, 2021 10:08
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.

5 participants