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

Update Thift Pool metrics to use class qualname #805

Merged
merged 4 commits into from
Apr 10, 2024

Conversation

selfcommit
Copy link
Contributor

@selfcommit selfcommit commented Sep 15, 2023

💸 TL;DR

Copied and updated from @ckwang8128:
The __name__ directive will use Iface as the pool label for every single thrift pool defined for a service, since the Thrift compiler uses Client as the class.

We likely want __qualname__ instead to get a more identifiable label for these pool metrics.

📜 Details

Going forward the label will change from IFace to the full object reference.
Example:
Iface -> TestThriftContextFactory.context_factory.<locals>.Iface

🧪 Testing Steps / Validation

I've locally run docker-compose run baseplate make test and updated the thrift tests to reflect the change.

✅ Checks

  • CI tests (if present) are passing
  • Adheres to code style for repo
    - [ ] Contributor License Agreement (CLA) completed if not a Reddit employee

@selfcommit selfcommit requested a review from a team as a code owner September 15, 2023 17:04
Copy link
Contributor

@ckw4ng ckw4ng left a comment

Choose a reason for hiding this comment

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

bless

@bjk-reddit bjk-reddit requested a review from chriskuehl April 8, 2024 13:28
@bjk-reddit bjk-reddit added the v2.7 label Apr 8, 2024
@chriskuehl chriskuehl merged commit e87aceb into develop Apr 10, 2024
3 checks passed
@chriskuehl chriskuehl deleted the doboyle_thrift_cls branch April 10, 2024 16:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

5 participants