Skip to content

Conversation

@fmaste
Copy link

@fmaste fmaste commented Oct 27, 2025

Description

reasonably detailed description of the pull request

Checklist

Quality

  • Commit sequence makes sense and have useful messages, see ref.
  • New tests are added and existing tests are updated.
  • Self-reviewed the PR.

Maintenance

  • Linked an issue or added the PR to the current sprint of ouroboros-network project.
  • Added labels.
  • Updated changelog files.
  • The documentation has been properly updated, see ref.

crocodile-dentist and others added 16 commits July 24, 2025 12:13
A trace is emitted whenever a hot outbound peer is demoted
or closed (possibly due to an error), giving the duration in
seconds of how long the peer has been in hot mode.

Added hot connection durations to peer selection debug state, which
traces durations upon receiving sigusr1 interrupt.
…n-durations

restore tracking of hot connection durations when demoting a peer
@github-project-automation github-project-automation bot moved this to In Progress in Ouroboros Network Oct 27, 2025
@fmaste fmaste force-pushed the fmaste/dmq-node-traces branch 3 times, most recently from e1a6963 to 4aaeabe Compare October 27, 2025 20:15
@fmaste fmaste force-pushed the fmaste/dmq-node-traces branch from 4aaeabe to 817dbff Compare October 27, 2025 20:28
hs-source-dirs: traces
visibility: public
exposed-modules:
Network.Mux.Traces
Copy link
Collaborator

Choose a reason for hiding this comment

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

We already have a very similar module Network.Mux.Trace, what about Network.Mux.Trace.TraceDispatcher?

-Wredundant-constraints
-Wunused-packages

library traces
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
library traces
library trace-dispatcher-instances

if flag(txsubmission-delay)
cpp-options: -DTXSUBMISSION_DELAY

library traces
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
library traces
library trace-dispatcher-instances

visibility: public
hs-source-dirs: traces
exposed-modules:
Ouroboros.Network.Traces
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
Ouroboros.Network.Traces
Ouroboros.Network.Trace.TraceDispatcher

-Wredundant-constraints
-Wno-unticked-promoted-constructors

library traces
Copy link
Collaborator

Choose a reason for hiding this comment

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

In main, ouroboros-network-framework was integrated with ouroboros-network package, when you'll be rebasing, it might be a good idea to just put all the instances in a single package in ouroboros-network, even in a single module, since here there's just one orphan instance.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe better place is cardano-diffusion package?

Copy link
Collaborator

Choose a reason for hiding this comment

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

It won't work if it is needed in dmq-node.

Copy link
Contributor

@crocodile-dentist crocodile-dentist Dec 4, 2025

Choose a reason for hiding this comment

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

We depend on cardano-diffusion in the dmq-node so we could get these instances from there, and keep ouroboros-network free from trace-dispatcher in case someone wants to use their own favorite tracing library with it? [edit] In my mind, things related to trace-dispatcher are cardano-specific, and ouroboros-network should be generic.

Copy link
Collaborator

Choose a reason for hiding this comment

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

We depend on cardano-diffusion

We don't, at least it main, and that's good, ref.

Copy link
Contributor

Choose a reason for hiding this comment

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

I use connectTo from Cardano.Network.NodeToClient to establish a local connection to the Cardano node.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Then, no objection from me to put these traces in cardano-diffusion.

-Wredundant-constraints
-Wunused-packages

library traces
Copy link
Contributor

Choose a reason for hiding this comment

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

There's also an option to make this a separate package for eg. cardano-network-mux-trace since network-mux in principle is a very general library. Another benefit is that in case we make changes to the tracers we will not have to make a release of network-mux. What's your take @coot ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Good point.

if flag(txsubmission-delay)
cpp-options: -DTXSUBMISSION_DELAY

library traces
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't cardano-diffusion be a better home for these instances?

Copy link
Collaborator

Choose a reason for hiding this comment

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

It won't work if these instances are also needed in dmq-node.

-Wredundant-constraints
-Wno-unticked-promoted-constructors

library traces
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe better place is cardano-diffusion package?

@coot
Copy link
Collaborator

coot commented Dec 4, 2025

current location suggested location argument
network-mux:cardano-logging ouroboros-network:mux-logging we don't want cardano-tracer dependency in network-mux, since we plan to publish it on hackage as an independent library at some point
ouroboros-network:cardano-logging OK -
ouroboros-network:framework-cardano-logging OK -

@crocodile-dentist do you agree?

@coot
Copy link
Collaborator

coot commented Dec 8, 2025

I agree with @crocodile-dentist that we can simply put all these trace instances in a single sublibrary in cardano-diffusion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: In Progress

Development

Successfully merging this pull request may close these issues.

4 participants