-
Notifications
You must be signed in to change notification settings - Fork 80
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 support for thrift service name in server traces #686
base: master
Are you sure you want to change the base?
Conversation
thriftbp/server.go
Outdated
} | ||
|
||
return fmt.Sprintf("%s.%s", | ||
filepath.Base(value.PkgPath()), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do we use filepath
here? filepath
is the one that uses /
for *nix and \
for windows, I don't think that's how go package path works. it should always be /
(as go package path is kind of url based) so we should use path
instead.
also do you have an example/test what this is supposed to get? I'm not sure why we infer this from go package path instead of set explicitly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call on filepath. Ill push that fix. I added a test to show what the expected output is. Im pulling the last part of the go path because that is usually defined by either go package set in the thrift file or the name of the thrift file itself. I then append the name of the processor with the processor suffix stripped since the thrift compiler matches that to the service schema name. Its not perfect. But I think its going to give us more consistent results. Especially since we can do something similar in the thrift client. This option should get us the closest to proper package and service names like grpc. But Im open to alternatives.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks @marcoferrer
This should align us with the upcoming baseplate.go changes. reddit/baseplate.go#686
Closes #
💸 TL;DR
Right now server trace spans for thrift report the service name as unknown. Because the thrift compiler doesn't generate schema metadata, we can fallback to relying on compiler conventions to source the service name defined in the schema.
📜 Details
Design Doc
Jira
🧪 Testing Steps / Validation
✅ Checks