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

Updated data_dog_trace/tracing to reflect awaited span attributes #4951

Closed
wants to merge 7 commits into from

Conversation

vpellan
Copy link

@vpellan vpellan commented May 14, 2024

Why

This PR changes the span attributes for Datadog trace/tracing module to comply with our backend

What

  • Changes the resource name, type, and tags for both data_dog_trace and data_dog_tracing
  • Added a new trace method specific to data_dog_trace to factorise the new code

Additional notes

This PR will be updated with the tests from our side
Other PRs will be created for 2.1.x, 2.0.x and 1.13.x backport

@rmosolgo
Copy link
Owner

Hi! Thanks for the suggestion.

our backend

What backend is this? Is this something to fix integration with DataDog's servers, or is this something to work better with your application?

I ask because this will be a breaking change for anyone who's currently using this tracer. Their dashboards will break because the tags they're watching will suddenly have no data. For that reason we'd have to be very careful about how this is rolled out (if it's released at all).

@vpellan
Copy link
Author

vpellan commented May 15, 2024

What backend is this?

I'm sorry I should have been clearer. I am a Datadog employee so I am talking about Datadog's backend. And "tests from our side" meant tests on dd-trace-rb repo.

The current implementation is broken and does not follow our (Datadog) specification. Our specification is similar to OpenTelemetry's one. For example, API Catalog requires the correct GraphQL span attributes and tags to work (and currently it does not)

We are currently working on fixing and communicating about breaking changes to affected organisations.

@rmosolgo
Copy link
Owner

Ok, that sounds great -- thanks for sharing that context. I definitely want to have a first-class DataDog tracer for GraphQL-Ruby.

I googled for the DataDog GraphQL spec but couldn't find it. Could you share a link to it? I'm curious to learn more about how it should work...

For compatibility's sake, could you please put the new code in a new trace module? For example, GraphQL::Tracing::DataDogAPICatalogTrace? Then, over the next few versions, we can:

  • Add an option for DataDogTrace to delegate to the new module when api_catalog: true (or something) is given
  • Include api_catalog: true in the docs so that new users get that behavior
  • In a later version, add a deprecation warning, telling users to add the api_catalog: true option if they aren't already using it
  • Then in another later version, replace the old DataDogTrace module with the new one, and add a warning to remove the (now-useless) api_catalog: true option.

I think an approach like that would set up new users to start on the right foot while allowing existing users to migrate over at their own pace. What do you think of that approach?

@vpellan
Copy link
Author

vpellan commented May 16, 2024

Hello !

Unfortunately the Datadog GraphQL spec is private, but it is similar to the OpenTelemetry one (https://opentelemetry.io/docs/specs/semconv/graphql/graphql-spans/)
You can also look at the implementation in other languages tracer (go: https://github.com/DataDog/dd-trace-go/tree/main/contrib/graphql-go/graphql, nodejs: https://github.com/DataDog/dd-trace-js/tree/master/packages/datadog-plugin-graphql/src)

I like this approach, but after discussing this issue with other Datadog Ruby developers, we may end up hosting the new trace module on dd-trace-rb repo. If we ever change our minds, we will reopen this PR. Thank you !

@vpellan vpellan closed this May 16, 2024
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.

None yet

2 participants