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

Add separate proc-macro-attribute definition crate for metrics #1898

Open
wants to merge 12 commits into
base: develop
from

Conversation

@dymayday
Copy link
Collaborator

dymayday commented Nov 21, 2019

PR summary

This PR add the definition of proc-macro-attribute as a separate crate in order to be used by the metric crate.
It needs to be put in a separate crate because currently, the same crate cannot export rule macros and attribute macros in the same time.

error: `proc-macro` crate types cannot export any items other than functions tagged with `#[proc_macro_derive]` currently

Help is welcome to find a way to properly instantiate a metrics's publisher.

changelog

  • if this is a code change that effects some consumer (e.g. zome developers) of holochain core, then it has been added to our between-release changelog with the format
- summary of change [PR#1234](https://github.com/holochain/holochain-rust/pull/1234)

documentation

@dymayday dymayday requested a review from struktured Nov 21, 2019
@dymayday dymayday self-assigned this Nov 21, 2019
Copy link
Collaborator

struktured left a comment

Looks good, not sure if latency is descriptive enough name. Maybe something like publish_latency. Also the default name of the metric should module_name.function_name or module_name.obj_name.method_name.

Regarding how to get a latency publisher, I think we need to assume or inject a singleton into your macro system via an init function or something. Will think this part over a bit. I would add @maackle as a reviewer too since he wants to integrate these features across tracing and otherwise.

@dymayday dymayday requested a review from maackle Nov 25, 2019
@dymayday dymayday changed the title WIP Add separate proc-macro-attribute definition crate for metrics Add separate proc-macro-attribute definition crate for metrics Nov 26, 2019
Copy link
Member

maackle left a comment

Nice macro 😎 I have two small suggestions and also a question for you and @struktured about how to test this.

lazy_static! {
pub static ref PUBLISHER: Arc<RwLock<crate::logger::LoggerMetricPublisher>> =
Arc::new(RwLock::new(crate::logger::LoggerMetricPublisher));
}

Comment on lines +33 to +37

This comment has been minimized.

Copy link
@maackle

maackle Nov 26, 2019

Member

I would suggest making this thread_local assuming that the LoggerMetricPublisher has no state that needs to be synced across threads. Then we don't have to use any Mutex, and can use a RefCell instead, which I think is important since we want the metric publisher to do as little as possible so it can reliably record metrics.

It's a simple change: PUBLISHER.write().unwrap() just becomes PUBLISHER.borrow_mut()

#[test]
fn main_test() {
eprintln!("Ahoy !");
}
Comment on lines +12 to +15

This comment has been minimized.

Copy link
@maackle

maackle Nov 26, 2019

Member

Is there any way to make this into an actual test? Or is the metric publisher not configurable to specify where things get published to? @struktured

use syn::{parse_macro_input, parse_quote, Attribute, FnArg, Ident, ItemFn, PatType, ReturnType};

#[proc_macro_attribute]
pub fn latency(args: TokenStream, input_function: TokenStream) -> TokenStream {

This comment has been minimized.

Copy link
@maackle

maackle Nov 26, 2019

Member

I kind of agree with @struktured that this could be more descriptive. I also like publish_latency.

Suggested change
pub fn latency(args: TokenStream, input_function: TokenStream) -> TokenStream {
pub fn publish_latency(args: TokenStream, input_function: TokenStream) -> TokenStream {
Copy link
Member

lucksus left a comment

Looks good, and I concure with @maackle's change requests. But happy to see this merged as soon as those are addressed.

@lucksus lucksus removed the NEEDS REVIEW label Dec 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.