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 metadata to metrics #380

Merged
merged 12 commits into from
Jul 15, 2023
Merged

Add metadata to metrics #380

merged 12 commits into from
Jul 15, 2023

Conversation

hlbarber
Copy link
Contributor

@hlbarber hlbarber commented Jun 6, 2023

Motivation

Closes #370

Changes

  • Add &Metadata argument to the Recorder::register_* methods
  • Add target: * and Level::* arguments to macros.
    • target defaults to module_path!()
    • Level defaults to Level::INFO.

TODO

  • Add documentation
  • Add info_counter!, warn_counter!, etc
  • Improve test coverage

@hlbarber
Copy link
Contributor Author

hlbarber commented Jun 6, 2023

Posted this in a draft state to garner comments before polishing it up.

@tobz
Copy link
Member

tobz commented Jun 7, 2023

@hlbarber High-level thoughts: this is pretty straightforward, but the macros should probably look more like the tracing macros.

In and of itself, Metadata feels as straightforward as I imagined it to be, but the lack of macro examples and the copied approach of how the describe_* macros handle detecting the unit parameter have solidified my thoughts around the macros needing to handle this better and in a way that's entirely compatible with the current macros. This is separate from actually providing level-prefixed variants of the macros, which I'm still a little hesitant about.

Essentially, what I'd want to see is:

  • target and level require a struct field notation in the macro arguments (like tracing)
  • metric name is always the first argument
  • parse the remaining arguments as either the "fixed function" target or level fields, or as labels (label parsing logic stays as-is)
  • cannot specify target or level after we've parsed something label-like (set a flag once we see the first non-target/level arg, basically)

Or with examples:

register_counter!("metric_name");
register_counter!("metric_name", "label_a" => "...");
register_counter!("metric_name", target: "overridden");
register_counter!("metric_name", level: Level::TRACE, &some_vec_of_labels);
register_counter!("metric_name", target: "overridden", level: Level::Info, "service" => "foo");

Ultimately, seeing this PR, I'd say that you could keep going with everything else on your list but hold off on the info_counter! stuff. If you're willing to take a crack at the procedural macro changes I've proposed above, that'd be good to. Otherwise, I would likely submit those as changes directly to your PR in the next week or two.

@hlbarber
Copy link
Contributor Author

hlbarber commented Jun 7, 2023

tracing::span and tracing::event, in order, accepts:

  • Optional target: target: {target}
  • Required level: Level::{LEVEL}
  • Required name: "{name}" (for span!)

This implementation, in order, accepts:

  • Optional target: target: {target}
  • Optional level: Level::{LEVEL} (for backwards compatibility)
  • Required metrics name: "{name}"
  • ...
register_counter!("metric_name");
register_counter!("metric_name", "label_a" => "...");
register_counter!(target: "overridden", "metric_name");
register_counter!(Level::TRACE, "metric_name", &some_vec_of_labels);
register_counter!(target: "overridden", Level::Info, "metric_name", "service" => "foo");

target and level require a struct field notation in the macro arguments

I can change level: Level::{LEVEL} or make Level::{LEVEL} required (like tracing) - either of these will avoid the "unit parameter approach" (which I agree should perhaps be avoided). I think the struct field notation helps for optional fields.

Side note: Perhaps the unit parameter should be struct field notation too? Maybe this allows these macros to be done declaratively rather than procedurally.

metric name is always the first argument

Can do, but this seems like a departure tracing where

macro_rules! span {
    (target: $target:expr, parent: $parent:expr, $lvl:expr, $name:expr) => { ... };
    (target: $target:expr, parent: $parent:expr, $lvl:expr, $name:expr, $($fields:tt)*) => { ... };

@tobz
Copy link
Member

tobz commented Jun 7, 2023

I can change level: Level::{LEVEL} or make Level::{LEVEL} required (like tracing) - either of these will avoid the "unit parameter approach" (which I agree should perhaps be avoided). I think the struct field notation helps for optional fields.

Right, I want level to use the struct field notation because it it's optional.

Side note: Perhaps the unit parameter should be struct field notation too? Maybe this allows these macros to be done declaratively rather than procedurally.

The hard-coded bits of the describe_*! macros are going away with the rework to metric "attributes" -- #337 (comment) -- so I would just ignore that for the moment.

Can do, but this seems like a departure tracing where

Fair point. I wasn't thinking we'd necessarily mirror everything about how the tracing macros do it, but I suppose all named optionals having to come first would make parsing easier, especially if we attempted to rewrite the macros to be declarative.

I'm also not against doing that either -- proc macros are heavy -- but it might need to be staged as an improvement that comes after this and metrics attributes, because those two items get rid of the grossest parts of the procedural macros, which is the wishy-washy handling of specific positional arguments (i.e. is this arg a Unit? let's check some hardcoded paths).

Let's stick with the current tracing-inspired approach for now.

Copy link
Member

@tobz tobz left a comment

Choose a reason for hiding this comment

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

After sitting down and going over this, I feel a lot better about the nature of the change and what we're adding.

Left two small comments.

metrics/src/metadata.rs Show resolved Hide resolved
metrics/src/metadata.rs Outdated Show resolved Hide resolved
@hlbarber hlbarber marked this pull request as ready for review June 19, 2023 16:48
Copy link
Member

@tobz tobz left a comment

Choose a reason for hiding this comment

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

Left one little doc comment thing, and it looks like this needs to have cargo fmt run on it, but overall very close. 👍🏻

/// Contains the following:
///
/// - A [`target`](Metadata::target), specifying the part of the system where the metric event occurred. When
/// initialized via the [metrics macro](metrics_macro), and left unspecified, this defaults to the module path the
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 needs to be changed to something like [metrics macros][metrics_macros] and then add [metrics_macros]: https://docs.rs/metrics/latest/metrics/#macros at the bottom of the overall doc comment.

I don't think there's a way to do intradoc links with just a crate name, but it's also cleaner/more intuitive, I think, to link the macros section of the metrics crate since that's where people will be consuming them from.

@tobz
Copy link
Member

tobz commented Jun 30, 2023

I'm going to try and take a look, in a few days, at just finalizing the remaining requested changes myself since it's so close.

@hlbarber
Copy link
Contributor Author

hlbarber commented Jul 13, 2023

"Rust / feature-check" is throwing a note: /usr/bin/ld: final link failed: No space left on device, any suggestions?

Copy link
Member

@tobz tobz left a comment

Choose a reason for hiding this comment

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

LGTM.

The feature check CI step has been finicky lately with "out of disk space" errors cropping up a lot. I just check this stuff manually before cutting releases and it's usually fine, so just gonna ignore it for now.

@tobz tobz merged commit 0193688 into metrics-rs:main Jul 15, 2023
jvimal-eg added a commit to edgeguard-dev/metrics that referenced this pull request Dec 24, 2023
* fix prometheus metric name and label key sanitizer (metrics-rs#296)

Co-authored-by: Toby Lawrence <tobz@users.noreply.github.com>

* metrics-util: add ability to collect metrics on a per-thread basis via DebuggingRecorder (metrics-rs#299)

Signed-off-by: Toby Lawrence <toby@nuclearfurnace.com>

* (cargo-release) version 0.12.1

* Improve handling of the global recorder instance (metrics-rs#302)

This gets rid of the dangerous `static mut`, adds more comments
about the code, relaxes the orderings and documents the unsoundness
of the `clear` function, in addition to marking it unsafe.

It implements a small once_cell-like abstraction.

Co-authored-by: Toby Lawrence <toby@nuclearfurnace.com>

* Fix `metrics::Cow` provenance issue (metrics-rs#303)

* Quantile Remapping Fix (metrics-rs#304)

* update changelogs prior to release

* (cargo-release) version 0.19.0

* (cargo-release) version 0.13.0

* (cargo-release) version 0.11.0

* (cargo-release) version 0.10.0

* Update CHANGELOG.md

* Update README.md

* Update ci.yml

* Update ci.yml

* Add RollingSummary to prevent summary saturation (metrics-rs#306)

* Update quanta requirement from 0.9.3 to 0.10.0 (metrics-rs#301)

Updates the requirements on [quanta](https://github.com/metrics-rs/quanta) to permit the latest version.
- [Release notes](https://github.com/metrics-rs/quanta/releases)
- [Changelog](https://github.com/metrics-rs/quanta/blob/main/CHANGELOG.md)
- [Commits](metrics-rs/quanta@v0.9.3...v0.10.0)

---
updated-dependencies:
- dependency-name: quanta
  dependency-type: direct:production
...

Signed-off-by: dependabot[bot] <support@github.com>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Document CI enforced msrv in readme and rust-version fields (metrics-rs#311)

* Change description to be SharedString (metrics-rs#312)

* Update hashbrown requirement from 0.11 to 0.12 (metrics-rs#266)

Updates the requirements on [hashbrown](https://github.com/rust-lang/hashbrown) to permit the latest version.
- [Release notes](https://github.com/rust-lang/hashbrown/releases)
- [Changelog](https://github.com/rust-lang/hashbrown/blob/master/CHANGELOG.md)
- [Commits](rust-lang/hashbrown@v0.11.0...v0.12.0)

---
updated-dependencies:
- dependency-name: hashbrown
  dependency-type: direct:production
...

Signed-off-by: dependabot[bot] <support@github.com>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Toby Lawrence <tobz@users.noreply.github.com>

* Shims remaining AtomicU64 usage (metrics-rs#313)

* Update parking_lot requirement from 0.11 to 0.12 (metrics-rs#268)

Updates the requirements on [parking_lot](https://github.com/Amanieu/parking_lot) to permit the latest version.
- [Release notes](https://github.com/Amanieu/parking_lot/releases)
- [Changelog](https://github.com/Amanieu/parking_lot/blob/master/CHANGELOG.md)
- [Commits](Amanieu/parking_lot@0.11.0...0.12.0)

---
updated-dependencies:
- dependency-name: parking_lot
  dependency-type: direct:production
...

Signed-off-by: dependabot[bot] <support@github.com>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Toby Lawrence <tobz@users.noreply.github.com>

* update changelogs

* (cargo-release) version 0.13.1

* add std atomics handle support back + changelogs

* (cargo-release) version 0.20.0

* (cargo-release) version 0.14.0

* (cargo-release) version 0.11.0

* changelog

* (cargo-release) version 0.12.0

* (cargo-release) version 0.7.0

* update changelog

* (cargo-release) version 0.6.0

* update changelog

* (cargo-release) version 0.20.1

* Remove incorrect return info (metrics-rs#316)

* Add a `KeyName` argument to `LabelFilter::should_include_label` (metrics-rs#342)

* rewind

* (cargo-release) version 0.13.0

* Use std sync primitives instead of parking_lot. (metrics-rs#344)

* Use std::sync::Mutex instead of parking_lot::Mutex.
* Bump MSRV to 1.60.0 for CI.

Co-authored-by: Toby Lawrence <tobz@users.noreply.github.com>

* clean up github CI workflow + rust-toolchain.toml to match quanta

* update portable-atomic to 1.0

* bump to prost 0.11 + fix spelling issue in metrics-tracing-context

* bump MSRV to 1.61 + update hashbrown/ahash deps

* update termion/ordered-float and pin predicates-* to avoid stupid 1.64 MSRV

* update syn

* update quanta + clean up semver notation in cargo.toml

* cleanup README wording for MSRV policy

* Add white background to splash image (metrics-rs#348)

* Install protoc in CI.

* fix spelling error in CI workflow

* Update ci.yml

* no need to run CI against macOS/Windows specifically

* Bring the metrics-observer protobufs up to date (metrics-rs#345)

* Use global paths for macros (metrics-rs#358)

* fix changes to fully qualified metrics crate ref change in macros

* update changelog

* update tui to 0.19

* bump numpy dep

* impl std::error::Error for metrics_exporter_tcp::Error

* changelog

* tweak test to avoid unused code

* rework 32 vs 64-bit arch atomics support + a lot of import consolidation/cleanup

* const-ify some stuff + rewrite some comments for the global recorder init code

* clean up clippy lints

* bump MSRV to 1.61.0

* (cargo-release) version 0.7.0

* (cargo-release) version 0.21.0

* (cargo-release) version 0.15.0

* (cargo-release) version 0.14.0

* (cargo-release) version 0.8.0

* (cargo-release) version 0.12.0

* allow publishing

* (cargo-release) version 0.2.0

* make it publishable pt 2

* push-gateway support authentication (metrics-rs#366)

* update changelog + fix failing feature check test

* (cargo-release) version 0.12.1

* feat(util): new helper type for recovering recorder after installing it (metrics-rs#362)

* Update aho-corasick to 1.0.

* Impl From<std::borrow::Cow> for KeyName (metrics-rs#378)

* changelog

* Add `Borrow` impl to `KeyName` (metrics-rs#381)

* bump deps + clippy stuff

* changelog

* pin hashbrown to avoid MSRV bump

* (cargo-release) version 0.15.1

* (cargo-release) version 0.21.1

* Add metadata to metrics (metrics-rs#380)

* add https support in Prometheus gateway (metrics-rs#392)

* migrate from procedural to declarative macros (metrics-rs#386)

* Make `Unit` methods return `&'static str` where possible (metrics-rs#393)

* simplify macros (metrics-rs#394)

* Add support for `Arc<T>` to `metrics::Cow<'a, T>`. (metrics-rs#402)

* Add support for `tracing::Span::record`ed fields in `metrics-tracing-context` (metrics-rs#408)

* fix(prom): `RollingSummary` overflow panic (metrics-rs#423)

* update CHANGELOG

* (cargo-release) version 0.12.2

* Remove unneeded unsafe from test (metrics-rs#427)

* Fix feature check in CI (metrics-rs#428)

* update changelogs/release notes

* fix unsafe/incorrect crossbeam-epoch usage in Block<T>

* Add a clippy CI check (metrics-rs#416) (metrics-rs#417)

* Try other resolved addresses if the first one fails (metrics-rs#429)

* update changelog

* Update quanta requirement from 0.11 to 0.12 (metrics-rs#396)

Updates the requirements on [quanta](https://github.com/metrics-rs/quanta) to permit the latest version.
- [Changelog](https://github.com/metrics-rs/quanta/blob/v0.12.0/CHANGELOG.md)
- [Commits](metrics-rs/quanta@v0.11.0...v0.12.0)

---
updated-dependencies:
- dependency-name: quanta
  dependency-type: direct:production
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Toby Lawrence <tobz@users.noreply.github.com>

* Update ordered-float requirement from 3.4 to 4.2 (metrics-rs#421)

Updates the requirements on [ordered-float](https://github.com/reem/rust-ordered-float) to permit the latest version.
- [Release notes](https://github.com/reem/rust-ordered-float/releases)
- [Commits](reem/rust-ordered-float@v3.4.0...v4.2.0)

---
updated-dependencies:
- dependency-name: ordered-float
  dependency-type: direct:production
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* fix quantile api

* missing clear

* reintroduce old way to avoid big refactor

---------

Signed-off-by: Toby Lawrence <toby@nuclearfurnace.com>
Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: Shaoyuan CHEN <chensy20@mails.tsinghua.edu.cn>
Co-authored-by: Toby Lawrence <tobz@users.noreply.github.com>
Co-authored-by: Toby Lawrence <toby@nuclearfurnace.com>
Co-authored-by: nils <48135649+Nilstrieb@users.noreply.github.com>
Co-authored-by: Dan Wilbanks <78116928+dfwilbanks395@users.noreply.github.com>
Co-authored-by: Daniel Nelson <daniel@wavesofdawn.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Lucas Kent <rubickent@gmail.com>
Co-authored-by: Fredrik Enestad <fredrik@enestad.com>
Co-authored-by: Christopher Hunt <huntchr@gmail.com>
Co-authored-by: zohnannor <35764628+zohnannor@users.noreply.github.com>
Co-authored-by: Sinotov Aleksandr <bratsinot@gmail.com>
Co-authored-by: Jacob Kiesel <kieseljake@live.com>
Co-authored-by: C J Silverio <ceejceej@gmail.com>
Co-authored-by: CinchBlue <aytasato@gmail.com>
Co-authored-by: JasonLi <lijingxuan92@126.com>
Co-authored-by: Mostafa Elhemali <mostafa.elhemaly@gmail.com>
Co-authored-by: Harry Barber <106155934+hlbarber@users.noreply.github.com>
Co-authored-by: Qingwen Zhao <864593343@qq.com>
Co-authored-by: david-perez <d@vidp.dev>
Co-authored-by: Lucio Franco <luciofranco14@gmail.com>
Co-authored-by: Nicolas Stinus <nicolas.stinus@gmail.com>
Co-authored-by: Valeriy V. Vorotyntsev <valery.vv@gmail.com>
ijc added a commit to ijc/metrics-exporter-statsd that referenced this pull request Jan 8, 2024
metrics-rs/metrics@metrics-v0.21.1...metrics-v0.22.0

There are some breaking changes:

- metrics-rs/metrics#414 Added a generic recorder to
  `SetRecorderError` and removed `set_boxed_recorder`
- metrics-rs/metrics#380 Added metadata to the metrics
- metrics-rs/metrics#394 Reworked the macros
mnpw pushed a commit to mnpw/metrics that referenced this pull request Mar 8, 2024
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.

Consider adding richer metadata to metric emission.
2 participants