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

[WIP] Add Counted Macro #1972

Closed

Conversation

manuelgdlvh
Copy link

@manuelgdlvh manuelgdlvh commented Jul 28, 2024

Fixes #
Design discussion issue (if applicable) #

Changes

I am developing a macros crate designed to streamline the generation of observability-related code across various types of metrics and logging functionalities. The initial focus of this crate is on implementing a basic counter, but our goal is to support a wide range of observability features

The crate leverages Rust's powerful attribute macros to simplify the configuration and integration process. By using these attribute macros, developers can avoid repetitive boilerplate code and configure observability tools with maximum flexibility. This approach ensures that the observability code remains concise, readable, and maintainable, allowing developers to focus more on their core application logic.

Key benefits of this macros crate include:

Reduction of Boilerplate Code: By abstracting common patterns and setups into macros, we significantly reduce the amount of boilerplate code developers need to write and maintain.

Consistency: The macros enforce consistent implementation of observability features across different parts of the codebase, reducing the likelihood of errors and inconsistencies.

Flexibility: The attribute macros are designed to be highly configurable, allowing developers to customize the generated code to fit their specific needs without sacrificing simplicity.

First steps looks like (LazyLock would be changed to ensure version compatibility):

Screenshot from 2024-07-29 00-07-26

Merge requirement checklist

  • CONTRIBUTING guidelines followed
  • Unit tests added/updated (if applicable)
  • Appropriate CHANGELOG.md files updated for non-trivial, user-facing changes
  • Changes in public API reviewed (if applicable)

@manuelgdlvh manuelgdlvh requested a review from a team July 28, 2024 22:01
Copy link

linux-foundation-easycla bot commented Jul 28, 2024

CLA Signed


The committers listed above are authorized under a signed CLA.

Copy link

codecov bot commented Jul 28, 2024

Codecov Report

Attention: Patch coverage is 0% with 97 lines in your changes missing coverage. Please review.

Project coverage is 74.5%. Comparing base (cd59346) to head (6c651c3).

Files Patch % Lines
opentelemetry-macros/src/metrics/counted.rs 0.0% 90 Missing ⚠️
opentelemetry-macros/src/lib.rs 0.0% 7 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##            main   #1972     +/-   ##
=======================================
- Coverage   74.9%   74.5%   -0.4%     
=======================================
  Files        122     124      +2     
  Lines      20404   20501     +97     
=======================================
- Hits       15289   15288      -1     
- Misses      5115    5213     +98     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@cijothomas cijothomas left a comment

Choose a reason for hiding this comment

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

There is definitely an interest if offering macros for not just metrics, but also other things. #1879

I am also particularly interested in using it to solve some perf issues like #1642 by leveraging macros.

We are not quite ready with a plan on how to make this happen in this repo. Until then, this is best hosted in opentelemetry-rust-contrib repo. Once it grows/stabilizes, it can be re-evaluated for bringing to core api/sdk itself.

Would you like to first describe the design (rough one) in an issue in the contrib repo, so we can discuss more?

@manuelgdlvh
Copy link
Author

manuelgdlvh commented Jul 29, 2024

Hi @cijothomas ,

It seems correct to me to include these first steps in the contrib crate.

I am currently working on the metrics part, more specifically on the counters part, and when it's finished, I plan to create a macro for Timer metrics that generates count and sum of times (I am open to hearing other priorities or needs to start with those).

Regarding what you mentioned about KeyValue, the first implementation is intended for const labels whose performance could be improved with static values, avoiding the repetitive creation of these structures.

For dynamic labels, we would need to change the way these values are passed, but I currently don't see how using a procedural macro could improve performance without changing the internal implementation of this part (which is not initially intended to be covered by these feature/s).

Within this first week/s, I would like to have the Counted macro ready, with the described design and implementation, a roadmap of possible interesting macros for the future, and to look into the performance issues you mentioned in depth and how to handle them efficiently, both dynamic and static.

Thanks for your time!

@cijothomas
Copy link
Member

don't see how using a procedural macro could improve performance

It won't improve perf of the actual SDK. I was referring to avoiding the un-necessary overhead when OTel SDK is not even enabled in this issue : #1642

@cijothomas
Copy link
Member

Closing the PR in this repo, as it was agreed this will be on contrib repo.

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.

2 participants