[core] Stop sending service information #738
Conversation
|
Great! Yeah, |
|
Indeed Also, looking forward to much cleaner code thanks to that change. It looks much better already! |
|
@pawelchcki and @palazzem I updated to remove all usage of Would love another review. |
| def write(trace, services) | ||
| def write(trace, services = nil) | ||
| unless services.nil? | ||
| Datadog::Patcher.do_once('SyncWriter#write') do |
There was a problem hiding this comment.
I am not sure this does 100% what I expect it to... seems like it, but /shrug
There was a problem hiding this comment.
Based on
dd-trace-rb/lib/ddtrace/patcher.rb
Line 24 in 42be529
It will do the thing in block only once.
Not necessarily an intended use case of this but if threads are of no concern it should be good to go.
There was a problem hiding this comment.
Yeah, it is definitely the point to only do it once. Mainly because Ruby doesn't have the rate limited logger yet, and if we happened to miss a place in code or someone is doing this manually, then it could get super spammer.
I was mostly concerned about calling this method statically instead of as an instance method.
There was a problem hiding this comment.
Yeah it should work, however its not thread safe so using it in this particular class might lead to unexpected results. Its only logs so at worst they will be sent 1 or 2 times more than expected.
Also SyncWriter itself should be highly discourage from use. It has some very unpleasant side effects in multithreaded environment
There was a problem hiding this comment.
👍 yeah, as long as we dedupe a bit, even once per-thread is fine by me, as long as it is 1M messages per-thread 😆
There was a problem hiding this comment.
Generally don't think calling Datadog::Patcher.do_once is a good idea; the state is effectively turned into global state, which has some really bad side effects (particularly for things like tests.) The Datadog::Patcher module was meant to be composed in, not be called directly.
We should probably compose in Datadog::Patcher and use it that way, or extract do_once into another module with a better name, then compose that in here.
| @@ -17,11 +17,9 @@ class Settings < Contrib::Configuration::Settings | |||
| lazy: true | |||
|
|
|||
| option :orm_service_name | |||
| option :service_name, depends_on: [:tracer] do |value| | |||
| (value || Utils.adapter_name).tap do |service_name| | |||
| tracer.set_service_info(service_name, Ext::APP, Datadog::Ext::AppTypes::DB) | |||
| def write(trace, services) | ||
| def write(trace, services = nil) | ||
| unless services.nil? | ||
| Datadog::Patcher.do_once('SyncWriter#write') do |
There was a problem hiding this comment.
Based on
dd-trace-rb/lib/ddtrace/patcher.rb
Line 24 in 42be529
It will do the thing in block only once.
Not necessarily an intended use case of this but if threads are of no concern it should be good to go.
|
Side note: we'll want to rebase #628 on these changes which currently has services still a part of the transport. |
We no longer need to make calls to
/servicesso these unnecessary calls can be removed from the tracer.Reviewers: The changes I made should all be internal and cause no breaking changes to user's code, but would love feedback specifically in this area, if you notice something removed that could be a problem, let me know.