I know it existed before, but the fact that it does is more evidence that our layering here is just wrong 😢 I still think we need to refactor this significantly to accept the fact that there's a dependence of debugger on tracing. It shouldn't be intiailizing independently like this, as it's a recipe for mismatch. All for later of course, but it's becoming clearer and clearer

The TracerManager.Instance access is a big issue frankly. It could refer to a completely different TracerSettings object for example. Essentially, this DebuggerManager should be created as part of the TracerManager creation, and then it can just pass in the values you need. Again, something for a different PR that we can work on.

I have some suspicions that that is the source of the extra overhead we saw coming from the refactor. Essentially you're doing a bunch of one-time setup, on the startup hot path, even if the debugger is never used. I think we should try to do this work lazily (I started working on a PR to do that)

In case the symbol upload is disabled, or all 3 products (CO, ER and DI) are explicitly disabled, the symbols upload should be skipped. The thing is that if the features are not explicitly disabled, but empty, we will want to upload symbols as soon as possible even if they are not currently enabled, in case it is enabled remotely. Otherwise, the user may not see symbols until we upload them all. Note, that we don't start to upload symbols untill we got a rc request to do so.

Fixed in 79622fa
Let me know if you still want to to disable it completly untill we got the first rc request to enable it.

See also ddfb2c4

Let me know if you still want to to disable it completly untill we got the first rc request to enable it

I think we should do that, because otherwise we're doing a bunch of work (and impacting performance) that most of our customers will never benefit from. I think we should definitely be lazy for all of this setup wherever we can be, because it has a clear impact on our benchmarks, and for large apps that could end up being very significant

After bfa3f79b5c87db7e916afbdc9375dce6dfcf2ae0 symdb won't be on by default if di is explicitly disabled

won't be on by default if di is explicitly disabled

Yeah, the concern is that's basically never going to be the case, so it's effectively still always going to be on by default for customers

I'd suggest you rewrite this to be similar to the tracer version e.g.

And then remove all the other changes above, make _internalDynamicInstrumentationEnabled a bool and do this change:

- DynamicInstrumentationEnabled = config
+ _internalDynamicInstrumentationEnabled = config
    .WithKeys(ConfigurationKeys.Debugger.DynamicInstrumentationEnabled)
    .AsBool(false);

But then how can I distinguish between empty/null to explicit false? I need to know that to decide if a feature might be enabled in the future or not.

I need to know that to decide if a feature might be enabled in the future or not.

Remote configuration overrides the value anyway, so it can always be added in the future I think? And if not, then we have a layering problem and confusion issue across products :/

Does it make sense now? bfa3f79b5c87db7e916afbdc9375dce6dfcf2ae0

#7366 (comment)

do we need this comment or it's a left over?

It was there before and I'm not sure if should I delete it or not

why do we need this delay?

To let the DI background initialization to run theoritically so in case we have a bug in our enabling condition this test will fail.

We should not have any Task.Delay in tests, because it will make them flaky. I practically guarantee that this will take longer than 1s in some cases, because the CI machines run at basically 100% capacity.

I strongly suggest having a deterministic way to know whether initialization has completed. If you can't, that suggests we need to redesign the code - APIs should be testable by default (e.g. no static access too 😉)

please let's avoid using any Thread.Sleep or Task.Delay in tests.

Fixed in 5683a7c78bfbca902bc8eb61710df96ff20c78ad

I strongly suggest having a deterministic way to know whether initialization has completed

We have a way for that, but in this case we need the opposite. Anyway I prefer to leave it as is was before, we don't have to change it now.

since you lock only specific part, do we care if we end up calling instance.Initialize on DynamicInstrumentation instance while another thread tries to create/destroy it potentially? Do we have potential race here?

Fixed in 1d6bfc7

We shouldn't use CancellationTokenSource in paths that can be hit during shutdown, because it can cause crashes. We generally have to use TaskCompletionSource instead (even though it's a bit of a pain)

Thanks for pointing that. Fixed in f3d6f4c949964231419ab232c0b5939abb08ac8e

Let me know if you still want to to disable it completly untill we got the first rc request to enable it

I think we should do that, because otherwise we're doing a bunch of work (and impacting performance) that most of our customers will never benefit from. I think we should definitely be lazy for all of this setup wherever we can be, because it has a clear impact on our benchmarks, and for large apps that could end up being very significant

Given you always write this log (which I think is fine, as it mirrors the tracer version 👍), you can probably change the other logs that are used by tests to use this one, instead of writing additional logs

I think that we use today only this initialization log in tests (there ar emore but not related to initialization)
Dynamic Instrumentation is disabled. To enable it, please set DD_DYNAMIC_INSTRUMENTATION_ENABLED environment variable to 'true'.
We can change the test to use the configuration value but we still want this log (without this log).

nit: You're already writing this log elsewhere, we should remove one of them (this is a problem today, we write the same log twice)

Fixed in 9cc8537e8fce4bd242b9c5798bfc51c00d140e38