[receiver/datadog] add client info to datadogreceiver #22992
Conversation
27a138c
to
5e8b8ef
Compare
receiver/datadogreceiver/receiver.go
Outdated
| ddmux.HandleFunc("/v0.7/traces", ddr.handleTraces) | ||
| ddr.server.Handler = ddmux | ||
| if err := ddr.server.ListenAndServe(); err != http.ErrServerClosed { | ||
| if err := ddr.server.Serve(hln); err != http.ErrServerClosed { |
There was a problem hiding this comment.
Do you mind updating the error check here? It is somewhat out of scope of the PR but it be nice if we could do something like:
if err := dd.server.Serve(hln); err != nil && ! errors.Is(err, http.ErrServerClosed) {This way we can be a little more explicit and defensive.
| return &datadogReceiver{ | ||
| params: params, | ||
| config: config, | ||
| nextConsumer: nextConsumer, | ||
| server: &http.Server{ | ||
| ReadTimeout: config.ReadTimeout, | ||
| Addr: config.HTTPServerSettings.Endpoint, |
There was a problem hiding this comment.
Just to confirm, this is no longer needed because we are using the listener to bind the interface?
There was a problem hiding this comment.
Yes, This is where the endpoint is used: https://github.com/open-telemetry/opentelemetry-collector/blob/main/config/confighttp/confighttp.go#L222-L225
receiver/datadogreceiver/receiver.go
Outdated
| } | ||
| hln, err := ddr.config.HTTPServerSettings.ToListener() | ||
| if err != nil { | ||
| return fmt.Errorf("error configuring datadog receiver listener: %w", err) |
There was a problem hiding this comment.
I suspect that this error will be logged at either ERROR status or Error: %w so we could be overly verbose.
What do you think about changing the error to be: failed to create datadog listener: %w ?
receiver/datadogreceiver/receiver.go
Outdated
| ddmux, | ||
| ) | ||
| if err != nil { | ||
| return fmt.Errorf("error configuring datadog receiver http server: %w", err) |
There was a problem hiding this comment.
With the error wording, would you mind if we make it more concise?
I mostly worry that when we log it, the message will already say something like: Failed to start "datadog": %w, so what if it was to be failed to create server definition: %w ?
5e8b8ef
to
595cc5c
Compare
|
@MovieStoreGuy thanks for a review :) Updated the PR :) |
595cc5c
to
671cb98
Compare
…#22992) Fix add client context and use other http settings defined in config. I've used ToServer() and ToListener(), which is similiar to otlp http receiver. They add a bunch of common logic, such as handling client context.
Description:
Fix add client context and use other http settings defined in config.
I've used ToServer() and ToListener(), which is similiar to otlp http receiver. They add a bunch of common logic, such as handling client context.
Link to tracking Issue: #22991
Testing:
Documentation: