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

Remove child_of option for trace from options table in Getting Started guide #1059

Closed
wants to merge 1 commit into from

Conversation

DocX
Copy link
Contributor

@DocX DocX commented Jun 1, 2020

From the current (0.36.0) implementation the trace method always overrides value of child_of hence not respecting anything that may be passed by the user: https://github.com/datadog/dd-trace-rb/blob/v0.36.0/lib/ddtrace/tracer.rb#L260

Removing the line from the table to remove confusion and misleading documentation

From the current (0.36.0) implementation the `trace` method always overrides value of `child_of` hence not respecting anything that may be passed by the user: https://github.com/datadog/dd-trace-rb/blob/v0.36.0/lib/ddtrace/tracer.rb#L260

Removing the line from the table to remove confusion and misleading documentation
@DocX DocX requested a review from a team June 1, 2020 13:55
@DocX DocX changed the title Remove misleading child_of option for trace Remove misleading child_of option for trace in documented options table Jun 1, 2020
@DocX DocX changed the title Remove misleading child_of option for trace in documented options table Remove misleading child_of option for trace in options table documentation Jun 1, 2020
@DocX DocX changed the title Remove misleading child_of option for trace in options table documentation Remove misleading child_of option for trace from options table documentation Jun 1, 2020
@DocX DocX changed the title Remove misleading child_of option for trace from options table documentation Remove child_of option for trace from options table in Getting Started guide Jun 1, 2020
@delner
Copy link
Contributor

delner commented Jun 4, 2020

@DocX Can you clarify why you saw this is the case? It's not my understanding that we intended to change this.... want to make sure there isn't actually a bug here.

@delner delner added community Was opened by a community member core Involves Datadog core libraries labels Jun 4, 2020
@DocX
Copy link
Contributor Author

DocX commented Jun 4, 2020

Hi @delner thank you for response.

Please see the link to the code line above in the summary above. It shows that the :child_of parameter is not currently being respected.

That said, I don't know if that is intended or it is bug. If the parameter should be respected then the code is wrong IMO.

@delner
Copy link
Contributor

delner commented Jun 8, 2020

@DocX Looking at the history, it looks like trace has never respected :child_of as an option. I suspect this is because it was added to wrap around start_span which uses it, and trace is pretty much always used to auto-resolve context, hence options[:child_of] = call_context.

We probably could change this to options[:child_of] ||= call_context if we wanted to allow this. I haven't considered the side effects of doing so, or whether it'd allow trace to operate with child_of as intended; worth trying though.

@DocX
Copy link
Contributor Author

DocX commented Jun 12, 2020

@delner makes sense :) Given it is documented that it should work - I assume there is a valid case for that.

Even ourselves would benefit from it - we need to pass parent span across threads in some cases, and currently we do it with the context - which is maybe not ideal? If we could just pass the child_of to a wrapping span inside of the thread, it would seem to be cleaner and more straightforward.

But again, I don't have much context into the internals, so I cannot really tell you what is the thing to do. We just noticed the discrepancy between the docs and implementation, and it's up to you what is the best approach :)

Thank you

@DocX
Copy link
Contributor Author

DocX commented Jun 16, 2020

Fixing by #1082

@DocX DocX closed this Jun 16, 2020
@marcotc
Copy link
Member

marcotc commented Oct 7, 2020

I've created a formal feature proposal for first-class threading support: #1202

@ericmustin ericmustin modified the milestone: 0.42.0 Oct 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community Was opened by a community member core Involves Datadog core libraries
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants