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

Log when certain functions are called an excessive number of times #5903

Merged
merged 1 commit into from
Mar 13, 2024

Conversation

dlon
Copy link
Member

@dlon dlon commented Mar 5, 2024

Fix DES-683.


This change is Reviewable

@dlon dlon force-pushed the macos-cpu-debug branch from af1a7a7 to 0966b3f Compare March 6, 2024 13:59
@dlon dlon marked this pull request as ready for review March 6, 2024 14:02
@dlon dlon requested a review from MarkusPettersson98 March 6, 2024 14:02
Copy link
Contributor

@MarkusPettersson98 MarkusPettersson98 left a comment

Choose a reason for hiding this comment

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

Reviewed 4 of 7 files at r1, 5 of 5 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @dlon)


talpid-types/src/error.rs line 79 at r2 (raw file):

    #[macro_export]
    macro_rules! detect_flood {

I think a doc-comment would be very nice to have, so that if I see detect_flood! being used somewhere I can quickly bring up the documentation at the call site instead of having to go to this source to figure out its purpose 😊

Code quote:

    #[macro_export]
    macro_rules! detect_flood {

Copy link
Member Author

@dlon dlon left a comment

Choose a reason for hiding this comment

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

Reviewable status: 8 of 9 files reviewed, 1 unresolved discussion (waiting on @MarkusPettersson98)


talpid-types/src/error.rs line 79 at r2 (raw file):

Previously, MarkusPettersson98 (Markus Pettersson) wrote…

I think a doc-comment would be very nice to have, so that if I see detect_flood! being used somewhere I can quickly bring up the documentation at the call site instead of having to go to this source to figure out its purpose 😊

Done.

Copy link
Contributor

@MarkusPettersson98 MarkusPettersson98 left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

Copy link
Member

@faern faern left a comment

Choose a reason for hiding this comment

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

Reviewed 4 of 7 files at r1, 2 of 5 files at r2, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @dlon)


talpid-types/src/error.rs line 72 at r3 (raw file):

}

pub mod flood {

I'm not a fan of more code ending up in this crate. But not going to block it this time since I don't have a concrete better suggestion right now.


talpid-types/src/error.rs line 81 at r3 (raw file):

    /// period of `CALLS_INTERVAL`.
    ///
    /// The caller of this macro must have `once_cell` and `log` as dependencies.

Instead of declaring that the caller crate must have once_cel and log I think it would be cleaner if the crate defining the macro re-exports these crates and uses $crate::once_cell::... etc instead.

Implicitly the macro also relies on special versions of these crates. If log 0.5 is released one day, this macro might not be compatible.

The reexports can be tagged with #[doc(hidden)] since they are not supposed to be part of the "public" API of this crate.


talpid-types/src/error.rs line 86 at r3 (raw file):

        () => {{
            static FLOOD: ::once_cell::sync::Lazy<
                ::std::sync::Mutex<talpid_types::flood::DetectFlood>,

This is fragile against the calling crate importing talpid_types under a different name. Can we use $crate here also instead?


talpid-types/src/error.rs line 91 at r3 (raw file):

            });
            if FLOOD.lock().unwrap().bump() {
                ::log::debug!("Flood: {}, line {}, col {}", file!(), line!(), column!());

Good log message! But should we maybe consider bumping this to at least a warning? I know we add it to debug something, but if this log line is ever executed it means the process is in a pretty bad state, so it's really a warning.


talpid-types/src/error.rs line 120 at r3 (raw file):

                let was_less = self.counter < CALLS_THRESHOLD;
                self.counter = self.counter.saturating_add(1);
                return was_less && self.counter >= CALLS_THRESHOLD;

A comment here would be justified IMHO. It took me a few iterations of thinking before I realized why we check the counter against the threshold twice.. Because we only want to return true once, right?

Can't this be simplified to just self.counter == CALLS_THRESHOLD to only return true exactly when we hit the threshold?


talpid-types/src/error.rs line 122 at r3 (raw file):

                return was_less && self.counter >= CALLS_THRESHOLD;
            }
            false

IMHO it would be slightly more straight forward if this was moved into the if branch. That removes the early return. The false is only ever hit when we hit the if branch anyway.

@dlon dlon force-pushed the macos-cpu-debug branch from c42d3bf to 57298ae Compare March 8, 2024 16:34
Copy link
Member Author

@dlon dlon left a comment

Choose a reason for hiding this comment

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

Reviewable status: 6 of 9 files reviewed, 4 unresolved discussions (waiting on @faern and @MarkusPettersson98)


talpid-types/src/error.rs line 72 at r3 (raw file):

Previously, faern (Linus Färnstrand) wrote…

I'm not a fan of more code ending up in this crate. But not going to block it this time since I don't have a concrete better suggestion right now.

Same. If we don't mind crate inflation, I could create talpid-log or talpid-debug.


talpid-types/src/error.rs line 81 at r3 (raw file):

Previously, faern (Linus Färnstrand) wrote…

Instead of declaring that the caller crate must have once_cel and log I think it would be cleaner if the crate defining the macro re-exports these crates and uses $crate::once_cell::... etc instead.

Implicitly the macro also relies on special versions of these crates. If log 0.5 is released one day, this macro might not be compatible.

The reexports can be tagged with #[doc(hidden)] since they are not supposed to be part of the "public" API of this crate.

Done.


talpid-types/src/error.rs line 86 at r3 (raw file):

Previously, faern (Linus Färnstrand) wrote…

This is fragile against the calling crate importing talpid_types under a different name. Can we use $crate here also instead?

Done.


talpid-types/src/error.rs line 120 at r3 (raw file):

Previously, faern (Linus Färnstrand) wrote…

A comment here would be justified IMHO. It took me a few iterations of thinking before I realized why we check the counter against the threshold twice.. Because we only want to return true once, right?

Can't this be simplified to just self.counter == CALLS_THRESHOLD to only return true exactly when we hit the threshold?

Done.


talpid-types/src/error.rs line 122 at r3 (raw file):

Previously, faern (Linus Färnstrand) wrote…

IMHO it would be slightly more straight forward if this was moved into the if branch. That removes the early return. The false is only ever hit when we hit the if branch anyway.

Done.

Copy link
Member

@faern faern left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 3 files at r4.
Reviewable status: 7 of 9 files reviewed, 4 unresolved discussions (waiting on @dlon and @MarkusPettersson98)


talpid-types/Cargo.toml line 20 at r4 (raw file):

thiserror = { workspace = true }
zeroize = "1.5.7"
log = "0.4"

log is also available as a workspace dependency.


talpid-types/src/error.rs line 81 at r3 (raw file):

Previously, dlon (David Lönnhager) wrote…

Done.

Macro documentation not updated yet.


talpid-types/src/error.rs line 92 at r4 (raw file):

                ::std::sync::Mutex<talpid_types::flood::DetectFlood>,
            > = $crate::flood::once_cell::sync::Lazy::new(|| {
                ::std::sync::Mutex::new($crate::flood::DetectFlood::default())

Mutex::new is a const fn. If we change last_clear: Instant to last_clear: Option<Instant> we can have a const fn DetectFlood::newand not need to deal with all the lazy stuff at all. It could just be a regular static. Would that not be simpler? At least it would get rid of needingonce_cell. It would slightly complicate bump`, but I think it can be done without much trouble.

What do you think? I think that would be somewhat simpler.


talpid-types/src/error.rs line 129 at r4 (raw file):

            } else {
                self.counter = self.counter.saturating_add(1);
                self.counter == CALLS_THRESHOLD

If we move the log::warn! call into here, we do not need to re-export log to make it available to the caller, since it would then not be part of the macro.

Then the DetectFlood would be coupled to the logging, but that's what it will only ever be used for, so does it matter? 🤔

Copy link
Member

@faern faern left a comment

Choose a reason for hiding this comment

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

Reviewable status: 7 of 9 files reviewed, 3 unresolved discussions (waiting on @dlon and @MarkusPettersson98)


talpid-types/src/error.rs line 129 at r4 (raw file):

Previously, faern (Linus Färnstrand) wrote…

If we move the log::warn! call into here, we do not need to re-export log to make it available to the caller, since it would then not be part of the macro.

Then the DetectFlood would be coupled to the logging, but that's what it will only ever be used for, so does it matter? 🤔

Aah, wait. I realize we depend on file!, line! etc to be inserted in the call position. Darn. They could theoretically be sent in as arguments, but it makes the solution dirtier. Let's ignore this, keeping the logging in the macro is fine!

Copy link

linear bot commented Mar 12, 2024

@mullvad mullvad deleted a comment from linear bot Mar 12, 2024
Copy link
Member Author

@dlon dlon left a comment

Choose a reason for hiding this comment

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

Reviewable status: 6 of 9 files reviewed, 2 unresolved discussions (waiting on @faern and @MarkusPettersson98)


talpid-types/Cargo.toml line 20 at r4 (raw file):

Previously, faern (Linus Färnstrand) wrote…

log is also available as a workspace dependency.

Done.


talpid-types/src/error.rs line 81 at r3 (raw file):

Previously, faern (Linus Färnstrand) wrote…

Macro documentation not updated yet.

Done.


talpid-types/src/error.rs line 92 at r4 (raw file):

Previously, faern (Linus Färnstrand) wrote…

Mutex::new is a const fn. If we change last_clear: Instant to last_clear: Option<Instant> we can have a const fn DetectFlood::newand not need to deal with all the lazy stuff at all. It could just be a regular static. Would that not be simpler? At least it would get rid of needingonce_cell. It would slightly complicate bump`, but I think it can be done without much trouble.

What do you think? I think that would be somewhat simpler.

Done!

Copy link
Member

@faern faern left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 3 of 3 files at r5, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved


talpid-types/src/error.rs line 92 at r4 (raw file):

Previously, dlon (David Lönnhager) wrote…

Done!

NICE! The get_or_insert there really made in sparkling ❤️

@dlon dlon force-pushed the macos-cpu-debug branch from 0e338e3 to 3e30376 Compare March 13, 2024 16:26
@dlon dlon force-pushed the macos-cpu-debug branch from 3e30376 to f739f1f Compare March 13, 2024 16:34
@dlon dlon merged commit 6113902 into main Mar 13, 2024
46 of 47 checks passed
@dlon dlon deleted the macos-cpu-debug branch March 13, 2024 16:47
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.

3 participants