-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Platform Tracing: fallback transaction name #3778
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
Platform Tracing: fallback transaction name #3778
Conversation
bd47bab to
0fc5cfb
Compare
|
@rmosolgo FYI I am finding that the test suite hangs on v1.13 when I run it locally with So perhaps I am missing a libev dependency and it doesn't handle it so gracefully. So for now I am targetting v1.12 (actually, we have not yet upgraded to v1.13 so I would prefer to target v1.12 either way, but we can live on a fork until v1.13 if necessary). |
rmosolgo
left a comment
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.
Looking good so far! If you don't mind adding some test for this behavior, that'd be great. Do you plan to use the .call feature? If not, I'd rather leave it out until someone actually needs it. (Just more for me to maintain, otherwise 😅 )
it hangs
Oh! I found that GitHub actions failed when libev_scheduler was installed, too. Maybe that was why.
| if selected_op | ||
| op_type = selected_op.operation_type | ||
| op_name = selected_op.name || "anonymous" | ||
| elsif fallback_name = fallback_transaction_name(query.context) | ||
| return "GraphQL/#{fallback_name}" | ||
| else | ||
| op_type = "query" | ||
| op_name = "anonymous" | ||
| end | ||
| "GraphQL/#{op_type}.#{op_name}" |
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.
sorry, personally, I have a hard time with multi-return methods, because I end up forgetting the different "exits" that the method has. if you don't mind:
| if selected_op | |
| op_type = selected_op.operation_type | |
| op_name = selected_op.name || "anonymous" | |
| elsif fallback_name = fallback_transaction_name(query.context) | |
| return "GraphQL/#{fallback_name}" | |
| else | |
| op_type = "query" | |
| op_name = "anonymous" | |
| end | |
| "GraphQL/#{op_type}.#{op_name}" | |
| txn_name = if selected_op | |
| op_type = selected_op.operation_type | |
| op_name = selected_op.name || "anonymous" | |
| "#{op_type}.#{op_name}" | |
| elsif (fallback_name = fallback_transaction_name(query.context)) | |
| fallback_name | |
| else | |
| "query.anonymous" | |
| end | |
| "GraphQL/#{txn_name}" |
8a511c1 to
74d0992
Compare
You made me do what I should have from the beginning: measured :D I wanted to avoid a SHA / SHA2 digest generation when it wouldn't be used. In practice, this is so close to instant its not worth the complexity If I am not mistaken, that's 0.2ms. Will remove that change. And incorporate your other comment.
This is next on my to do, I have left some empty tests for now so we can agree on scope. Do these seem reasonable to you? One for my snowflake implementation in Datadog, and one for the general purpose implementation, in a tracer that relies on that implementation |
rmosolgo
left a comment
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.
Yep, this scope LGTM 😎
af77ff9 to
7fcaf4a
Compare
7fcaf4a to
778fc88
Compare
Enable a developer to provide the platform trace API a fallback name to use when a query name is not provided. Note that this is a *fallback*, the query name is preferred when available. Testing is limited to Datadog and New Relic. Support is within any tracer that delegates to `PlatformTracing#transaction_name`, which appears to include AppSignal and Scout. Not sure if that is exhaustive.
778fc88 to
9e93fe4
Compare
|
I have implemented the specs and got it working on master rather than 1.12, ready for review! |
rmosolgo
left a comment
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 for this improvement!
| describe GraphQL::Tracing::DataDogTracing do | ||
| module DataDogTest | ||
| class Query < GraphQL::Schema::Object | ||
| add_field GraphQL::Types::Relay::NodeField |
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.
| add_field GraphQL::Types::Relay::NodeField | |
| include GraphQL::Types::Relay::HasNodeField |
There was a bug here fixed in master since this branch came off: #3791
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.
I'll merge this and fix it on master
Enable a developer to provide the platform trace API a fallback name to
use when a query name is not provided.
Note that this is a fallback, the query name is preferred when
available.
Testing is limited to Datadog and New Relic.
Support is within any tracer that delegates to
PlatformTracing#transaction_name, which appears to include AppSignaland Scout. Not sure if that is exhaustive.
Fixes #3769