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: metrics #103

Merged
merged 5 commits into from
Feb 16, 2023
Merged

feat: metrics #103

merged 5 commits into from
Feb 16, 2023

Conversation

HarryET
Copy link
Contributor

@HarryET HarryET commented Feb 15, 2023

Description

Adds more metrics as well as a macro for easily incrementing a counter on the state.metrics property when they are enabled.

How Has This Been Tested?

N/a, will test in grafana after deployed to prod and then PR the new dashboard back

Due Diligence

  • Breaking change
  • Requires a documentation update
  • Requires a e2e/integration test update

Comment on lines +1 to +11
#[macro_export]
macro_rules! increment_counter {
($state:ident.$property:ident, $metric:ident) => {{
use {opentelemetry::Context, tracing::debug};

if let Some(metrics) = &$state.$property {
metrics.$metric.add(&Context::current(), 1, &[]);
debug!("incremented `{}` counter", stringify!($metric));
}
}};
}
Copy link
Member

@chris13524 chris13524 Feb 15, 2023

Choose a reason for hiding this comment

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

thought: I was going to suggest using a function instead of a macro here, but looks like the problem with that is not being able to read the metric name from the metric object itself (for the debug!()). Interesting limitation of the library.

($state:ident.$property:ident, $metric:ident) => {{
use {opentelemetry::Context, tracing::debug};

if let Some(metrics) = &$state.$property {
Copy link
Member

Choose a reason for hiding this comment

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

Looks like this macro only supports a first argument in the form ident.ident? What if I wanted to pass a more deeply-nested identifier such as ident.ident.ident? Would using $metrics:expr as the first macro parameter make this more generalized?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It can be added but right now I know that it will only be one deep -> can be looked at later though

@HarryET HarryET merged commit 2e46761 into main Feb 16, 2023
@HarryET HarryET deleted the harry/feat/metrics branch February 16, 2023 14:46
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.

None yet

2 participants