Skip to content

Conversation

@gregbarasch
Copy link
Contributor

@gregbarasch gregbarasch commented Dec 23, 2025

💸 TL;DR

Our thrift server metrics assume that there will only be one server running within a single runtime. This is not always true. If a second server is running, the thrift server metrics will clash with one another. This PR adds a serverName label to our serverside metrics.

📜 Details

This update is backwards compatible, and notably should not increase cardinality in any significant way. Preexisting metric usage will emit "unknown" as the serverName, which will retain metric cardinality and will add a negligible amount of data for metric DBs.

The API itself does not enable backward compatible attribute changes. We have a choice. We can:

  1. inject the serverName into the ctx on each request
  2. Always call GetThriftServiceName(cfg.Processor) -- needs testing
  3. (The option chosen) Provide an alternative WithArgs function

Commonly, we might make use of the optionals or builder pattern for forward looking, backwards compatible changes. Instead though we've opted to use a Cfg struct, as this matches patterns found elsewhere in the repo.

🧪 Testing Steps / Validation

Unit tests

✅ Checks

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

Signed-off-by: Greg Barasch <gregbarasch@gmail.com>
@gregbarasch gregbarasch requested a review from a team as a code owner December 23, 2025 20:08
@gregbarasch gregbarasch requested review from konradreiche, mathyourlife-reddit and pacejackson and removed request for a team December 23, 2025 20:08
@gregbarasch gregbarasch marked this pull request as draft December 23, 2025 20:20
@gregbarasch gregbarasch marked this pull request as ready for review December 23, 2025 20:43
Copy link
Member

@konradreiche konradreiche left a comment

Choose a reason for hiding this comment

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

This makes sense to me but I believe we don't have the equivalent in baseplate v2. I've added @kylelemons for additional perspective.

Copy link
Contributor

@kylelemons kylelemons left a comment

Choose a reason for hiding this comment

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

We can't add additional labels here without breaking compatibility with thriftbp/v2 via interop, and unfortunately such breakages cause runtime metrics blackouts.

This should probably be brought up in #baseplate-spec to weigh the alternatives

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants