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

Use processor/client module name as thrift rpc service. #1073

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

trevorriles
Copy link
Contributor

The goal is to have both ends of the rpc class the same across frameworks, similar to how grpc operates.

This should align us with the upcoming baseplate.go changes.

reddit/baseplate.go#686

Closes #

💸 TL;DR

📜 Details

Design Doc

Jira

🧪 Testing Steps / Validation

✅ Checks

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

This should align us with the upcoming baseplate.go changes.
reddit/baseplate.go#686
@@ -220,12 +220,14 @@ def example(self, context):
thrift_client_span.get_span_context().span_id,
)

self.assertEqual(thrift_client_span.name, "example_service/example")
self.assertEqual(
thrift_client_span.name, "tests.integration.test_thrift.TestService/example"

Choose a reason for hiding this comment

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

So this contains the full python package name. To get something close to a "thrift" package, you'll need to only keep the last part of the package.

The assertion should be against test_thrift.TestService.

Choose a reason for hiding this comment

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

The python file name usually maps to the thrift schema name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good, is it always only a single depth package? I should be able to just split on . and take the last element.

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.

2 participants