feat: add ability to config profiling on/off

[nagmo-starkware]

Pull Request type

Please check the type of change your PR introduces:

• Bugfix
• Feature
• Code style update (formatting, renaming)
• Refactoring (no functional changes, no API changes)
• Build-related changes
• Documentation content changes
• Other (please describe):

What is the current behavior?

Issue Number: N/A

What is the new behavior?

Does this introduce a breaking change?

• Yes
• No

Other information

---

This change is 

[nagmo-starkware]

• feat: add ability to config profiling on/off #1867 👈
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @nagmo-starkware and the rest of your teammates on Graphite

[DvirYo-starkware]

Reviewable status: 0 of 6 files reviewed, 1 unresolved discussion (waiting on @nagmo-starkware and @yair-starkware)

a discussion (no related file):

1. It doesn't pass tests.
2. I have some questions about this PR, let's talk about this next week

---

[nagmo-starkware]

Reviewable status: 0 of 15 files reviewed, 1 unresolved discussion (waiting on @DvirYo-starkware and @yair-starkware)

a discussion (no related file):

Previously, DvirYo-starkware wrote…

1. It doesn't pass tests.
2. I have some questions about this PR, let's talk about this next week

Done.

---

[DvirYo-starkware]

Reviewed 4 of 6 files at r1, 6 of 7 files at r2, 4 of 5 files at r3, all commit messages.
Reviewable status: 14 of 15 files reviewed, 9 unresolved discussions (waiting on @nagmo-starkware and @yair-starkware)

a discussion (no related file):
Add a todo to configure this on runtime using the monitoring gw.
Add a todo remove this true-false thing when our metric exporter will support more advent filtering.
Add a todo to consider configuring with levels.

As we discussed, consider different configuration names, such as true and false.
Consider changing the location of this configuration value. In my opinion, it should be next to the collect_metrics value (maybe this value is not in the correct location).

---

---

config/default_config.json line 178 at r3 (raw file):

  },
  "profiling": {
    "description": "should the node track profiling information",

Should. The description in the config file starts with a capital letter

---

config/default_config.json line 178 at r3 (raw file):

  },
  "profiling": {
    "description": "should the node track profiling information",

Consider using the same format as for collect metrics ."If true, collect metrics for the node.".
In addition, this information is only a set of metrics, so consider changing information to metrics. Maybe change this name to collect_profilling_metrics

---

crates/papyrus_common/src/metrics.rs line 36 at r3 (raw file):

pub const PAPYRUS_NUM_ACTIVE_OUTBOUND_SESSIONS: &str = "papyrus_num_active_outbound_sessions";

/// global variable set by the main config to enable profiling.

Global. Capital letter
Consider explicitly writing this collect profiling metrics.

---

crates/papyrus_node/src/main.rs line 301 at r3 (raw file):

    }

    PROFILING_STATUS.get_or_init(|| config.profiling);

why not use set() instead of get_or_init(). This value should be initialized only here.

---

crates/papyrus_proc_macros/Cargo.toml line 16 at r3 (raw file):

metrics.workspace = true
metrics-exporter-prometheus.workspace = true
papyrus_common = { path = "../papyrus_common" }

Use the version of the crate like in other places where we use papyrus_common

---

crates/papyrus_proc_macros/src/lib.rs line 130 at r3 (raw file):

        })
        .collect::<Vec<_>>();
    let attr = parts

Give this variable a more explanatory name, also to the config variable.

---

crates/papyrus_proc_macros/src/lib.rs line 146 at r3 (raw file):

        {
            let mut start_function_time = None;
            if !#controll_with_config || (#controll_with_config && *(papyrus_common::metrics::PROFILING_STATUS.get().unwrap_or(&false))) {

Why not expect? This value should be a valid

[nagmo-starkware]

Reviewable status: 8 of 15 files reviewed, 9 unresolved discussions (waiting on @DvirYo-starkware and @yair-starkware)

---

config/default_config.json line 178 at r3 (raw file):

Previously, DvirYo-starkware wrote…

Should. The description in the config file starts with a capital letter

Done.

---

config/default_config.json line 178 at r3 (raw file):

Previously, DvirYo-starkware wrote…

Consider using the same format as for collect metrics ."If true, collect metrics for the node.".
In addition, this information is only a set of metrics, so consider changing information to metrics. Maybe change this name to collect_profilling_metrics

Done.

---

crates/papyrus_common/src/metrics.rs line 36 at r3 (raw file):

Previously, DvirYo-starkware wrote…

Global. Capital letter
Consider explicitly writing this collect profiling metrics.

Done.

---

crates/papyrus_node/src/main.rs line 301 at r3 (raw file):

Previously, DvirYo-starkware wrote…

why not use set() instead of get_or_init(). This value should be initialized only here.

Done.

---

crates/papyrus_proc_macros/Cargo.toml line 16 at r3 (raw file):

Previously, DvirYo-starkware wrote…

Use the version of the crate like in other places where we use papyrus_common

Done.

---

crates/papyrus_proc_macros/src/lib.rs line 130 at r3 (raw file):

Previously, DvirYo-starkware wrote…

Give this variable a more explanatory name, also to the config variable.

Done.

---

crates/papyrus_proc_macros/src/lib.rs line 146 at r3 (raw file):

Previously, DvirYo-starkware wrote…

Why not expect? This value should be a valid

yes it should.. but if for some reason it's not I don't see any compelling reason to panic.. worst case you won't get the metrics.
BTY an example to where it won't be valid is if you run something from another binary and not main.. since main sets it

[DvirYo-starkware]

Reviewed 7 of 7 files at r4, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @nagmo-starkware and @yair-starkware)

a discussion (no related file):

Previously, DvirYo-starkware wrote…

Add a todo to configure this on runtime using the monitoring gw.
Add a todo remove this true-false thing when our metric exporter will support more advent filtering.
Add a todo to consider configuring with levels.

As we discussed, consider different configuration names, such as true and false.
Consider changing the location of this configuration value. In my opinion, it should be next to the collect_metrics value (maybe this value is not in the correct location).

Consider again adding the todos

---

---

config/default_config.json line 178 at r3 (raw file):

Previously, nagmo-starkware wrote…

Done.

I don't see the changes.

---

crates/papyrus_node/src/config/mod.rs line 103 at r4 (raw file):

                "collect_profiling_metrics",
                &self.collect_profiling_metrics,
                "If true, collect metrics for the node.",

You need to update also the defualt config file.
Also, consider adding profiling. ie "If true, collect profiling metrics for the node."

[yair-starkware]

Reviewed 1 of 6 files at r1, 5 of 7 files at r2, 2 of 5 files at r3, 7 of 7 files at r4, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @DvirYo-starkware and @nagmo-starkware)

---

crates/papyrus_proc_macros/src/lib.rs line 104 at r4 (raw file):

/// This macro will emit a histogram metric with the given name and the latency of the function.
/// The macro also receives a boolean that controls if the metric should be controlled
/// by the profiling configuration value or not.

Suggestion: The macro also receives a boolean for whether it will be emitted only when profiling is activated or at all times

Code quote:

/// The macro also receives a boolean that controls if the metric should be controlled
/// by the profiling configuration value or not.

---

crates/papyrus_proc_macros/src/lib.rs line 127 at r4 (raw file):

        .map(|s| {
            TokenStream::from_str(s)
                .expect("attribute should include metric name and controll with config boolean")

Suggestion: Expecting metric name and bool (is for profiling only)

Code quote:

"attribute should include metric name and controll with config boolean

[nagmo-starkware]

Reviewable status: 5 of 15 files reviewed, 4 unresolved discussions (waiting on @DvirYo-starkware and @yair-starkware)

a discussion (no related file):

Previously, DvirYo-starkware wrote…

Consider again adding the todos

I don't really see the value of this todos. if at some point someone would like to improve this he can. the todos won't help in this case IMO.

---

---

crates/papyrus_node/src/config/mod.rs line 103 at r4 (raw file):

Previously, DvirYo-starkware wrote…

You need to update also the defualt config file.
Also, consider adding profiling. ie "If true, collect profiling metrics for the node."

Done.

[DvirYo-starkware]

Reviewed 10 of 10 files at r5, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @nagmo-starkware)

[DvirYo-starkware]

Reviewed 3 of 3 files at r6, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @nagmo-starkware)