feat: add optional name for activity descriptions #170

Anyitechs · 2024-03-08T23:54:48Z

This PR closes issue #127 and the implementation follows the conversation on the issue thread.

carlaKC

Conceptually looks good, a few comments. Feel free to pull out of draft once addressed.

sim-lib/src/lib.rs

Anyitechs · 2024-03-14T07:46:04Z

@carlaKC I made the requested changes. Let me know if this looks good now. Thank you.

enigbe

@Anyitechs Good job on the PR. Here are a few comments I have:

To make the new activity_name field even clearer for users, I'd recommend adding some documentation to the README. This could explain its optional nature and how it's handled in the code.
I noticed that the activity_name or index is used in logs related to activities and at most the simulation events (when they're sent to the events consumer). I'm curious about why we'd have a name/index prefix that doesn't extend to the simulation outputs and results as well. Shouldn't there be consistency across board? I don't think it will be easy to relate activity -> event -> output -> result logs if only the activities and the events have the prefix. I know this is out of the scope of the issue but happy to get your (as well as @carlaKC's) thoughts on this.

Anyitechs · 2024-03-15T07:40:49Z

@Anyitechs Good job on the PR. Here are a few comments I have:

Thank you.

To make the new activity_name field even clearer for users, I'd recommend adding some documentation to the README. This could explain its optional nature and how it's handled in the code.

Sure! I'll update the README in a seperate PR following this.

I noticed that the activity_name or index is used in logs related to activities and at most the simulation events (when they're sent to the events consumer). I'm curious about why we'd have a name/index prefix that doesn't extend to the simulation outputs and results as well. Shouldn't there be consistency across board? I don't think it will be easy to relate activity -> event -> output -> result logs if only the activities and the events have the prefix. I know this is out of the scope of the issue but happy to get your (as well as @carlaKC's) thoughts on this.

I agree @enigbe . I guess that's what @carlaKC referred to as "activity-related" logging above. I'll work on that and update the PR.

carlaKC · 2024-03-15T13:36:59Z

Sure! I'll update the README in a seperate PR following this.

Please update readme in the same PR, it's easy for things to get out of sync/be forgotten otherwise.

carlaKC

Shaping up!

I think that this still needs better handling for random activity - what we have right now will print odd looking lines.

README.md

carlaKC · 2024-03-25T15:20:27Z

sim-lib/src/lib.rs

@@ -819,32 +838,32 @@ async fn produce_events<N: DestinationGenerator + ?Sized, A: PaymentGenerator +
                let amount = match node_generator.payment_amount(capacity) {
                    Ok(amt) => {
                        if amt == 0 {
-                            log::debug!("Skipping zero amount payment for {source} -> {destination}.");
+                            log::debug!("{activity_name} activity => Skipping zero amount payment for {source} -> {destination}.");


nit: replace => with :, also won't this print activity if a name is not set?

Ok. I'll replace that.

And if a name isn't set, it should print the index of the activity. But I'll confirm this is true for this as well.

We don't set an activity_name for randomly generated activity (above, line 776) right?

Yes, we don't set an activity_name for randomly generated activity. I see your point on this logging activity if a name isn't set for it.

How best do you think we can handle this? I had a name set for it before but you earlier mentioned we should take it off.

By only logging the prefix if we've got a value there?

if self.activity_name != "" { format!("{} activity:", self.activity_name) } else {"".to_string()}

Could inline, pulled out into a impl of Payment, or leave activity_name as an option so that we can use some of the uwrap_or functionality.

Thank you for the clarification. I've addressed this and updated the PR.

sim-cli/src/main.rs

carlaKC

Needs a rebase on master!

carlaKC · 2024-04-09T15:47:17Z

sim-cli/src/main.rs

+        for validated_activity in &validated_activities {
+            if validated_activity.activity_name == act.activity_name {
+                anyhow::bail!(LightningError::ValidationError(
+                    "Duplicate activity name is not allowed".to_string(),


It would be helpful to provide the user with the name so they know what to fix?

carlaKC · 2024-04-09T15:48:43Z

sim-lib/src/lib.rs

+        if self.activity_name.is_empty() {
+            write!(
+                f,
+                "Processed {} payments sending {} msat total with {:.2}% success rate.",


Surely we can do this with fewer LOC?

carlaKC · 2024-04-09T15:49:28Z

sim-cli/src/main.rs

@@ -182,11 +182,20 @@ async fn main() -> anyhow::Result<()> {
            },
        };

+        for validated_activity in &validated_activities {


Build a hashmap like we do for alias and pubkey rather than iterating through every activity every time

carlaKC · 2024-04-09T15:50:31Z

README.md

-provide a set of nodes and the simulator will generate activity based 
-on the topology of the underlying graph. Note that payments will only 
-be sent between the `nodes` that are provided so that liquidity does 
+To run the simulator with random activity generation, you just need to


Please pull wholesale formatting changes out into their own commit

This still hasn't been addressed?

If you want to run all these formatting changes on the README, please run them first and then make your changes.

Anyitechs · 2024-04-11T16:01:14Z

Thank you for the detailed review @carlaKC. I've addressed all the feedbacks, rebased, and updated the PR as well.

Let me know if there's anything else I need to look at.

carlaKC

Looks like the functionality is mostly here (though I think we still need to clean up the logging for random activity).

On a higher level, the commit structure in this PR needs some work. Most of the changes can be squashed down to a single commit, and formatting changes / docs / unrelated fixups left in their own commit.

sim-lib/src/lib.rs

Anyitechs · 2024-04-22T23:37:14Z

Looks like the functionality is mostly here (though I think we still need to clean up the logging for random activity).

On a higher level, the commit structure in this PR needs some work. Most of the changes can be squashed down to a single commit, and formatting changes / docs / unrelated fixups left in their own commit.

Thank you. I've addressed the feedbacks and neatened up the commit structure.

carlaKC · 2024-04-23T19:22:02Z

Thank you. I've addressed the feedbacks and neatened up the commit structure.

Have you? The first commit is still a mix of formatting and logical changes, and has a dangling TODO comment.

To me, the logical commit structure would be:

Formatting changes to the README (or remove all of those)
Commit with actual rust changes
Commit with documentation updates for features

Right now everything is intermingled in the first commit - formatting changes to the readme, and rust code.

README.md

sim-lib/src/lib.rs

Anyitechs · 2024-04-24T14:20:36Z

Thank you. I've addressed the feedbacks and neatened up the commit structure.

Have you? The first commit is still a mix of formatting and logical changes, and has a dangling TODO comment.

To me, the logical commit structure would be:

Formatting changes to the README (or remove all of those)

Commit with actual rust changes

Commit with documentation updates for features

Right now everything is intermingled in the first commit - formatting changes to the readme, and rust code.

Thank you @carlaKC for taking the time to review this and providing feedbacks as well. I really appreciate it.

I've worked on the commit structure and it's currently being squashed into two commits:

The actual code implementation, and
The documentation update of the feature

Let me know if I missed anything. Thank you.

carlaKC

Thanks for updating the commit ordering!

Please try to test changes with a few standard inputs before you push/request review - there's a panic in this change.

The various logging issues that I've pointed out also become more apparent when you spend some time running the project and getting familiar. I know that these seem like frustrating nits, but when we're looking at a change like this which really is just aesthetic we really do need to get things right to justify changing the code at all.

sim-cli/src/main.rs

carlaKC · 2024-04-24T17:43:54Z

sim-cli/src/main.rs

@@ -182,13 +183,23 @@ async fn main() -> anyhow::Result<()> {
            },
        };

+        if activity_name_map.contains_key(&act.activity_name) {


When there are two activities with no activity_name set, this will detect that None is in the map twice, error out and then panic on unwrap.

I think it makes sense to put this whole block in if act.activity_name.IsSome() so that we're only adding items that actually have values. Using an Option as a key gets tricky (as seen here).

carlaKC · 2024-04-24T17:48:43Z

sim-lib/src/lib.rs

@@ -1105,7 +1136,8 @@ impl Display for PaymentResultLogger {
        let total_payments = self.success_payment + self.failed_payment;
        write!(
            f,
-            "Processed {} payments sending {} msat total with {:.2}% success rate.",
+            "{} Processed {} payments sending {} msat total with {:.2}% success rate.",


This line represents a summary of all payments - why are use using a single activity name here?

carlaKC · 2024-04-24T17:55:52Z

sim-lib/src/lib.rs

+                    "Starting {} activity producer for {}: {}.",
+                    executor.activity_name,


Prints: Starting activity for random payments, as above perhaps think about using this strictly as a prefix.

sim-lib/src/lib.rs

carlaKC · 2024-04-24T18:14:02Z

If it's useful to getting this in, we can also just re-add random activity as a name, but perhaps with an index. I don't like it but it's not really I hill I'm willing to die on - hopefully that simplifies all the logging.

carlaKC · 2024-05-06T19:00:13Z

Needs rebase!

carlaKC · 2024-05-13T12:24:35Z

Ping @Anyitechs any updated here?

Anyitechs · 2024-05-14T08:01:34Z

Ping @Anyitechs any updated here?

Yes, will update the PR by EOD.

Anyitechs · 2024-05-14T21:45:22Z

Ping @Anyitechs any updated here?

Yes, will update the PR by EOD.

I just updated the PR now.

carlaKC

Can be rebased on master to fix unrelated build failure.

I think that this change is high impact enough if we just have logging in produce_events, so final comment is to remove this entirely from PaymentResultLogger.

carlaKC · 2024-05-15T18:28:39Z

sim-lib/src/lib.rs

+            let total_payments = activity_result.success_payment + activity_result.failed_payment;
+            writeln!(
+                f,
+                "{} Processed {} payments sending {} msat total with {:.2}% success rate.",


The intent behind this comment was to remove the names from PaymentResultLogger completely - we don't want to print these results on a per-activity basis, it'll be too spammy where the whole point here is to give a very high level aggregate.

I believe we can also remove activity_name from Payment completely if we don't have this logging?

carlaKC · 2024-05-15T18:29:32Z

sim-lib/src/lib.rs

@@ -352,6 +358,16 @@ struct Payment {
    dispatch_time: SystemTime,
 }

+impl Payment {
+    fn activity_prefix(&self) -> String {


nit: add a comment explaining usage here / that this should be added before a log with no space

carlaKC · 2024-05-15T18:29:44Z

sim-lib/src/lib.rs

+impl Payment {
+    fn activity_prefix(&self) -> String {
+        if !self.activity_name.is_empty() {
+            format!("{} activity:", self.activity_name)


nit: should have a space after : otherwise it reads X activity:Payment when we use it below

carlaKC · 2024-05-15T18:30:24Z

sim-lib/src/lib.rs

            generators.push(ExecutorKit {
                source_info: node_info.clone(),
                network_generator: network_generator.clone(),
+                activity_name: format!("Index {index} random"),


nit: Random index {}

carlaKC · 2024-05-15T18:30:34Z

sim-lib/src/lib.rs

@@ -788,10 +812,11 @@ impl Simulation {
            network_generator.lock().await
        );

-        for (node_info, capacity) in active_nodes.values() {
+        for (index, (node_info, capacity)) in active_nodes.values().enumerate() {


nit: just use i, more idiomatic

…ture details

Anyitechs · 2024-05-23T12:05:29Z

Can be rebased on master to fix unrelated build failure.

I think that this change is high impact enough if we just have logging in produce_events, so final comment is to remove this entirely from PaymentResultLogger.

Thank you for taking the time to always review and give feedback on this PR @carlaKC. I've addressed all the comments and updated the PR. Let me know if there's anything I'm missing.

carlaKC

There are some superfluous changes leftover from previous iterations of this PR, and activity_name isn't being used in logs in produce_events in several places.

carlaKC · 2024-05-24T16:41:50Z

sim-lib/src/lib.rs

@@ -1083,7 +1098,7 @@ async fn produce_events<N: DestinationGenerator + ?Sized, A: PaymentGenerator +
                log::debug!("Generated payment: {source} -> {}: {amount} msat.", destination);

                // Send the payment, exiting if we can no longer send to the consumer.
-                let event = SimulationEvent::SendPayment(destination.clone(), amount);
+                let event = SimulationEvent::SendPayment(destination.clone(), amount,);


unnecessary comma after amount,

carlaKC · 2024-05-24T16:59:36Z

sim-lib/src/lib.rs

@@ -380,7 +384,7 @@ pub enum PaymentOutcome {
 }

 /// Describes a payment from a source node to a destination node.
-#[derive(Debug, Clone, Copy, Serialize)]


unnecessary change?

carlaKC · 2024-05-24T17:00:00Z

sim-lib/src/lib.rs

@@ -1241,14 +1256,14 @@ async fn produce_simulation_results(
                            SimulationOutput::SendPaymentSuccess(payment) => {
                                if let Some(source_node) = nodes.get(&payment.source) {
                                    set.spawn(track_payment_result(
-                                        source_node.clone(), results.clone(), payment, listener.clone()
+                                        source_node.clone(), results.clone(), payment.clone(), listener.clone()


Unnecessary clone here and 3x below?

carlaKC · 2024-05-24T17:01:38Z

sim-lib/src/lib.rs

@@ -1083,7 +1098,7 @@ async fn produce_events<N: DestinationGenerator + ?Sized, A: PaymentGenerator +
                log::debug!("Generated payment: {source} -> {}: {amount} msat.", destination);


Why not use activity_name here?

carlaKC · 2024-05-24T17:06:54Z

We've spent quite a lot of time on review for a purely aesthetic change, and I don't have much capacity left to see this to the finish line review-wise. If somebody has need for this feature and has the time to review I'll happily come back and give this a final pass + merge once it's been cleaned up.

carlaKC · 2024-06-18T12:58:45Z

Closing due to inactivity.

Anyitechs marked this pull request as ready for review March 8, 2024 23:55

Anyitechs marked this pull request as draft March 8, 2024 23:56

Anyitechs mentioned this pull request Mar 9, 2024

Feature: Optional name for activity descriptions #127

Open

carlaKC reviewed Mar 9, 2024

View reviewed changes

sim-lib/src/lib.rs Outdated Show resolved Hide resolved

sim-lib/src/lib.rs Outdated Show resolved Hide resolved

sim-lib/src/lib.rs Outdated Show resolved Hide resolved

Anyitechs marked this pull request as ready for review March 14, 2024 07:41

enigbe requested changes Mar 14, 2024

View reviewed changes

carlaKC reviewed Mar 25, 2024

View reviewed changes

Anyitechs requested review from enigbe and carlaKC April 7, 2024 00:12

carlaKC reviewed Apr 9, 2024

View reviewed changes

Anyitechs force-pushed the feat/optional-activity-name branch from 16fd20d to 6f92f3a Compare April 11, 2024 15:46

Anyitechs requested a review from carlaKC April 11, 2024 16:14

carlaKC reviewed Apr 22, 2024

View reviewed changes

sim-lib/src/lib.rs Outdated Show resolved Hide resolved

Anyitechs force-pushed the feat/optional-activity-name branch from 6f92f3a to c14ef99 Compare April 22, 2024 23:03

Anyitechs force-pushed the feat/optional-activity-name branch from c14ef99 to 62229d9 Compare April 23, 2024 00:01

Anyitechs commented Apr 23, 2024

View reviewed changes

README.md Show resolved Hide resolved

carlaKC reviewed Apr 23, 2024

View reviewed changes

sim-lib/src/lib.rs Outdated Show resolved Hide resolved

Anyitechs force-pushed the feat/optional-activity-name branch from 62229d9 to 0fa96c9 Compare April 24, 2024 14:08

carlaKC requested changes Apr 24, 2024

View reviewed changes

Anyitechs force-pushed the feat/optional-activity-name branch from 0fa96c9 to b2c27d7 Compare May 14, 2024 21:39

carlaKC requested changes May 15, 2024

View reviewed changes

Ifeanyichukwu added 2 commits May 23, 2024 12:55

feat: add optional name for activity descriptions

2968897

docs(README.md): update documentation with optional activity_name fea…

fe51df7

…ture details

Anyitechs force-pushed the feat/optional-activity-name branch from b2c27d7 to fe51df7 Compare May 23, 2024 11:58

carlaKC requested changes May 24, 2024

View reviewed changes

carlaKC closed this Jun 18, 2024

		"Starting {} activity producer for {}: {}.",
		executor.activity_name,

		@@ -1083,7 +1098,7 @@ async fn produce_events<N: DestinationGenerator + ?Sized, A: PaymentGenerator +
		log::debug!("Generated payment: {source} -> {}: {amount} msat.", destination);

feat: add optional name for activity descriptions #170

feat: add optional name for activity descriptions #170

Conversation

Anyitechs commented Mar 8, 2024

carlaKC left a comment

Choose a reason for hiding this comment

Anyitechs commented Mar 14, 2024

enigbe left a comment

Choose a reason for hiding this comment

Anyitechs commented Mar 15, 2024

carlaKC commented Mar 15, 2024

carlaKC left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlaKC left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Anyitechs commented Apr 11, 2024

carlaKC left a comment

Choose a reason for hiding this comment

Anyitechs commented Apr 22, 2024

carlaKC commented Apr 23, 2024 • edited Loading

Anyitechs commented Apr 24, 2024 • edited Loading

carlaKC left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlaKC commented Apr 24, 2024

carlaKC commented May 6, 2024

carlaKC commented May 13, 2024

Anyitechs commented May 14, 2024

Anyitechs commented May 14, 2024

carlaKC left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Anyitechs commented May 23, 2024

carlaKC left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlaKC commented May 24, 2024

carlaKC commented Jun 18, 2024

carlaKC commented Apr 23, 2024 •

edited

Loading

Anyitechs commented Apr 24, 2024 •

edited

Loading