profiler: use instrumentation telemetry #1348
Conversation
To avoid typos and for a little extra type-safety.
Not used by the API any more.
The profiler sets agentURL to http://host:port/profiling/v1/input. So doing path.Join on that to add the telemetry proxy endpoint is wrong for two reasons: The "//" at the beginning will be converted to a "/", and the endpoint will be wrong. We need to parse the URL and replace the path with the telemetry proxy endpoint.
|
@nsrip-dd Can you update the PR description with more details? Additionally, should this be associated with a milestone? |
In particular, explain what the default http.Client will be and why to use a non-default http.Client
There was a problem hiding this comment.
Thanks for the review @katiehockman! I'll see if I can come up with a reasonable unit test for this change.
| func TestTelemetryEnabled(t *testing.T) { | ||
| received := make(chan *telemetry.AppStarted, 1) | ||
| server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| if r.URL.Path != "/telemetry/proxy/api/v2/apmtelemetry" { |
There was a problem hiding this comment.
Maybe add a test where the URL is invalid to make sure it finishes gracefully?
The debug feature (which causes the backend to emit more verbose logging) is only intended for development. There's no point having it be a public member of the telemetry Client since it can be enabled via an environment variable for development.
|
Hello, I have been trying to track down the release that this change is included in. I managed to locate it in this diff: v1.40.1...v1.41.0#diff-84cf63f4014172e12f686aac850adafb3669dd237fe2f64267bd20cc5e2b1860 , but the corresponding release notes doesn't mention it: https://github.com/DataDog/dd-trace-go/releases/tag/v1.41.0 . Was this by choice to not mention it? If not, it would be much appreciated to have the release notes amended, thanks! |
|
Hi! At the time of this PR, we hadn't actually enabled instrumentation telemetry collection by default. It had to be manually enabled via an environment variable, which we were only doing for internal testing. Since there weren't any user-facing changes related to this, it wasn't mentioned in the notes. We didn't actually revisit telemetry until later, in v1.49.0, and the implementation changed quite a bit. But at that point, it was documented. Was/is this code causing issues? |
|
Hi @nsrip-dd , the code in question is not causing issues. My team and I detected an attempt to connect to the host |
Use the telemetry client from
internal/telemetryfor the profiler to report the profiler's environment (Go version, OS version, imported modules, etc.) and configuration (enabled profiles, profile rates, etc.) to the Datadog instrumentation telemetry backend.This PR starts with the client off by default so it can be selectively enabled in our testing/staging environment. It must be explicitly enabled with the environment variable
DD_INSTRUMENTATION_TELEMETRY_ENABLED=true.Also, we no longer need to set the
Typefield oftelemetry.Dependencysince that field is no longer part of the telemetry API.