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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove span_type method call on SpanOperation #4776

Merged

Conversation

TonyCTHsu
Copy link
Contributor

@TonyCTHsu TonyCTHsu commented Jan 10, 2024

Closes #4762

Why

Datadog tracer is removing a deprecated method span_type for dd-trace-rb 2+ . This is the PR.

What

  • Change the tracing implementation in graphql-ruby from span_type to type.
  • Remove constant reference from dd-trace-rb
  • Deprecate service option and its default
  • Deprecate tracer option

Additional Notes

The test is setup with DataDog/dd-trace-rb#3376

馃毃 This change is backward compatible (All graphql-ruby versions works with dd-trace-rb 1+ ). However, graphql-ruby users need to upgrade with this change to use dd-trace-rb 2+.

@TonyCTHsu TonyCTHsu marked this pull request as ready for review January 10, 2024 12:10
@@ -158,8 +155,7 @@ def resolve_type_lazy(object:, type:, query:)

def resolve_type_span(span_key, object, type, query)
platform_key = @platform_key_cache[DataDogTrace].platform_resolve_type_key_cache[type]
@tracer.trace(platform_key, service: @service_name) do |span|
span.span_type = 'custom'
@tracer.trace(platform_key, service: @service_name, type: 'custom') do |span|
Copy link
Owner

Choose a reason for hiding this comment

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

Just to make sure I understand:

What will dd-trace-rb do with this type: 'custom' argument? Will it ignore it, or 馃挜 ?

If this code only works with dd-trace-rb 2+, what do you think about adding a version check of some kind in def initialize above? If that's possible, it might be a nicer development experience than troubleshooting the argument error (if span: ... does cause an error, that is).

Copy link
Contributor Author

@TonyCTHsu TonyCTHsu Jan 16, 2024

Choose a reason for hiding this comment

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

What will dd-trace-rb do with this type: 'custom' argument?

The data model of Span/SpanOperation contains a type field, which can filled by multiple ways.

  1. Through Tracing#trace(...), there are both type/span_type as optional keyword arguments. https://github.com/DataDog/dd-trace-rb/blob/master/lib/datadog/tracing/tracer.rb#L131-L132

Both options yields the same outcome.

[2] pry(main)> span = Datadog::Tracing.trace('my_span', span_type: 'custom');
[3] pry(main)> span.type
=> "custom"
[4] pry(main)> span = Datadog::Tracing.trace('my_span', type: 'custom');
[5] pry(main)> span.type
=> "custom"
  1. Through attribute setter from the SpanOperation, https://github.com/DataDog/dd-trace-rb/blob/master/lib/datadog/tracing/span_operation.rb#L502
    Since, span_type= is just an alias for type=, the following code are the same.
[3] pry(main)> Datadog::Tracing.trace('my_span') do |span|
[3] pry(main)*   span.span_type = 'custom'
[3] pry(main)*   puts span.type
[3] pry(main)* end
custom
[3] pry(main)> Datadog::Tracing.trace('my_span') do |span|
[3] pry(main)*   span.type = 'custom'
[3] pry(main)*   puts span.type
[3] pry(main)* end
custom

If this code only works with dd-trace-rb 2+

This is not entirely correct. Because the changes basically migrate the deprecated usage to a supported usage, it works on all dd-trace-rb versions.

  • dd-trace-rb 1.x supports both span_type= and type=
  • dd-trace-rb 2.x only supports type=

The existing graphql-ruby versions without such changes won't work with dd-trace-rb 2+. That is why graphql-ruby users need to upgrade with this change to use dd-trace-rb 2+.

@rmosolgo rmosolgo added this to the 2.2.6 milestone Jan 24, 2024
@rmosolgo rmosolgo merged commit 3adaba5 into rmosolgo:master Jan 25, 2024
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Question related to upgrade
2 participants