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

feat(sidecar): gross tip revenue metric #339

Merged
merged 4 commits into from
Oct 29, 2024

Conversation

mempirate
Copy link
Contributor

@mempirate mempirate commented Oct 29, 2024

Closes #245

Copy link
Contributor

@thedevbirb thedevbirb left a comment

Choose a reason for hiding this comment

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

I've not seen the metric added anywhere in the sidecar code, is this PR still draft?

@@ -2,6 +2,9 @@ name: Bolt Sidecar CI

on:
push:
branches:
- unstable
- main
Copy link
Contributor

Choose a reason for hiding this comment

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

The main branch is unused. Any specific reason for adding it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We might use it in the future

Comment on lines +90 to +105
// If the tip is too large, we need to split it into multiple u64 parts
if tip > u64::MAX as u128 {
let mut parts = Vec::new();
while tip > u64::MAX as u128 {
parts.push(u64::MAX);
tip -= u64::MAX as u128;
}

parts.push(tip as u64);

for part in parts {
counter!(GROSS_TIP_REVENUE).increment(part);
}
} else {
counter!(GROSS_TIP_REVENUE).increment(tip as u64);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

In my opinion this is a bit simpler/succint and doesn't distinguish between edge cases and regular cases which is nice.

Suggested change
// If the tip is too large, we need to split it into multiple u64 parts
if tip > u64::MAX as u128 {
let mut parts = Vec::new();
while tip > u64::MAX as u128 {
parts.push(u64::MAX);
tip -= u64::MAX as u128;
}
parts.push(tip as u64);
for part in parts {
counter!(GROSS_TIP_REVENUE).increment(part);
}
} else {
counter!(GROSS_TIP_REVENUE).increment(tip as u64);
}
// Takes also into account tips larger than a u128 by splitting them into u64 parts
let mut parts = Vec::new();
let u64_max = u64::MAX as u128;
while tip > 0 {
// We can only add a max tip of u64::MAX.
let min = tip.min(u64_max) as u64;
parts.push(min);
// Subtract the part we just added, if something is left then tip is going to be
// greater than zero.
tip = tip.saturating_sub(u64_max)
}
parts.push(tip as u64);
for part in parts {
counter!(GROSS_TIP_REVENUE).increment(part);
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True, but this always does an allocation

Copy link
Contributor

Choose a reason for hiding this comment

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

That's fair, also because with an u64 you can express a tip up to 18.44 ETH :D

@mempirate mempirate marked this pull request as draft October 29, 2024 15:17
@mempirate
Copy link
Contributor Author

I've not seen the metric added anywhere in the sidecar code, is this PR still draft?

Yes mb, converted

@mempirate mempirate marked this pull request as ready for review October 29, 2024 15:57
Copy link
Contributor

@thedevbirb thedevbirb left a comment

Choose a reason for hiding this comment

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

Great!

@thedevbirb thedevbirb merged commit 7f76718 into unstable Oct 29, 2024
3 checks passed
@thedevbirb thedevbirb deleted the jonas/feat/gross-revenue-metric branch October 29, 2024 16:14
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.

Sidecar metrics for revenue
3 participants