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.

Sign up for GitHub

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

[exporter/datadogexporter] Rely on http.Client's timeout instead of in exporterhelper's #6414

Merged
merged 1 commit into from
Nov 30, 2021
Merged

[exporter/datadogexporter] Rely on http.Client's timeout instead of in exporterhelper's #6414

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions exporter/datadogexporter/factory.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,7 +154,8 @@ func createMetricsExporter(
cfg,
set,
pushMetricsFn,
exporterhelper.WithTimeout(cfg.TimeoutSettings),
// explicitly disable since we rely on http.Client timeout logic.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will this be confusing if some exporters use this TimeoutSettings as the timeout setting for the entire operation where as other exporters use it per network call? This makes me think we should have a different configuration option for it. What do you think?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have tried to use a pattern that is already used in the Collector in other exporters:

opentelemetry-collector-contrib/exporter/signalfxexporter/factory.go

Lines 121 to 122 in 243d642

// explicitly disable since we rely on http.Client timeout logic.
exporterhelper.WithTimeout(exporterhelper.TimeoutSettings{Timeout: 0}),

opentelemetry-collector-contrib/exporter/lokiexporter/factory.go

Lines 71 to 72 in 243d642

// explicitly disable since we rely on http.Client timeout logic.
exporterhelper.WithTimeout(exporterhelper.TimeoutSettings{Timeout: 0}),

opentelemetry-collector-contrib/exporter/splunkhecexporter/factory.go

Lines 105 to 106 in 243d642

// explicitly disable since we rely on http.Client timeout logic.
exporterhelper.WithTimeout(exporterhelper.TimeoutSettings{Timeout: 0}),

opentelemetry-collector-contrib/exporter/observiqexporter/exporter.go

Lines 42 to 43 in 243d642

// explicitly disable since we rely on http.Client timeout logic.
exporterhelper.WithTimeout(exporterhelper.TimeoutSettings{Timeout: 0}),

I would expect most users not to care about a global Consume[Metrics/Traces/Logs] function timeout (they don't even need to know that such a function exists to use the exporter). If we use different options I think we should have a wider conversation to have a consistent solution for all exporters that do this differently today.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, I'm happy to approve this PR. It would be good to have that discussion if the pattern of disabling this timeout already happening, maybe the global timeout is less important.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Opened open-telemetry/opentelemetry-collector#4497 to discuss this

exporterhelper.WithTimeout(exporterhelper.TimeoutSettings{Timeout: 0 * time.Second}),
exporterhelper.WithRetry(cfg.RetrySettings),
exporterhelper.WithQueue(cfg.QueueSettings),
exporterhelper.WithShutdown(func(context.Context) error {
Expand Down Expand Up @@ -207,7 +208,8 @@ func createTracesExporter(
cfg,
set,
pushTracesFn,
exporterhelper.WithTimeout(cfg.TimeoutSettings),
// explicitly disable since we rely on http.Client timeout logic.
exporterhelper.WithTimeout(exporterhelper.TimeoutSettings{Timeout: 0 * time.Second}),
exporterhelper.WithRetry(cfg.RetrySettings),
exporterhelper.WithQueue(cfg.QueueSettings),
exporterhelper.WithShutdown(func(context.Context) error {
Expand Down
2 changes: 1 addition & 1 deletion exporter/datadogexporter/internal/metadata/metadata.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,7 +154,7 @@ func pushMetadata(cfg *config.Config, buildInfo component.BuildInfo, metadata *H
req, _ := http.NewRequest(http.MethodPost, path, bytes.NewBuffer(buf))
utils.SetDDHeaders(req.Header, buildInfo, cfg.API.Key)
utils.SetExtraHeaders(req.Header, utils.JSONHeaders)
client := utils.NewHTTPClient(10 * time.Second)
client := utils.NewHTTPClient(cfg.TimeoutSettings)
mx-psi marked this conversation as resolved.
Show resolved Hide resolved
resp, err := client.Do(req)

if err != nil {
Expand Down
5 changes: 3 additions & 2 deletions exporter/datadogexporter/internal/utils/http.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import (
"time"

"go.opentelemetry.io/collector/component"
"go.opentelemetry.io/collector/exporter/exporterhelper"
)

var (
Expand All @@ -38,9 +39,9 @@ var (
)

// NewHTTPClient returns a http.Client configured with the Agent options.
func NewHTTPClient(timeout time.Duration) *http.Client {
func NewHTTPClient(settings exporterhelper.TimeoutSettings) *http.Client {
return &http.Client{
Timeout: timeout,
Timeout: settings.Timeout,
Transport: &http.Transport{
Proxy: http.ProxyFromEnvironment,
DialContext: (&net.Dialer{
Expand Down
2 changes: 1 addition & 1 deletion exporter/datadogexporter/metrics_exporter.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ func translatorFromConfig(logger *zap.Logger, cfg *config.Config) (*translator.T
func newMetricsExporter(ctx context.Context, params component.ExporterCreateSettings, cfg *config.Config) (*metricsExporter, error) {
client := utils.CreateClient(cfg.API.Key, cfg.Metrics.TCPAddr.Endpoint)
client.ExtraHeader["User-Agent"] = utils.UserAgent(params.BuildInfo)
client.HttpClient = utils.NewHTTPClient(10 * time.Second)
client.HttpClient = utils.NewHTTPClient(cfg.TimeoutSettings)
mx-psi marked this conversation as resolved.
Show resolved Hide resolved

utils.ValidateAPIKey(params.Logger, client)

Expand Down
6 changes: 3 additions & 3 deletions exporter/datadogexporter/trace_connection.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import (
"github.com/DataDog/datadog-agent/pkg/trace/exportable/stats"
"github.com/gogo/protobuf/proto"
"go.opentelemetry.io/collector/component"
"go.opentelemetry.io/collector/exporter/exporterhelper"

"github.com/open-telemetry/opentelemetry-collector-contrib/exporter/datadogexporter/internal/utils"
)
Expand All @@ -45,19 +46,18 @@ type traceEdgeConnectionImpl struct {
}

const (
traceEdgeTimeout time.Duration = 10 * time.Second
traceEdgeRetryInterval time.Duration = 10 * time.Second
)

// createTraceEdgeConnection returns a new traceEdgeConnection
func createTraceEdgeConnection(rootURL, apiKey string, buildInfo component.BuildInfo) traceEdgeConnection {
func createTraceEdgeConnection(rootURL, apiKey string, buildInfo component.BuildInfo, settings exporterhelper.TimeoutSettings) traceEdgeConnection {

return &traceEdgeConnectionImpl{
traceURL: rootURL + "/api/v0.2/traces",
statsURL: rootURL + "/api/v0.2/stats",
buildInfo: buildInfo,
apiKey: apiKey,
client: utils.NewHTTPClient(traceEdgeTimeout),
client: utils.NewHTTPClient(settings),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This changes the timeout value from 10 seconds to 15 seconds (if I read

opentelemetry-collector-contrib/exporter/datadogexporter/factory.go

Lines 49 to 53 in 77d48a5

func defaulttimeoutSettings() exporterhelper.TimeoutSettings {
return exporterhelper.TimeoutSettings{
Timeout: 15 * time.Second,
}
}
correctly), is that fine? Should we change defaulttimeoutSettings to return 10 seconds instead?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this is fine (we care about the timeout of the batch processor for traces but that's about it, the timeout here should be fine no matter the value)

}
}

Expand Down
2 changes: 1 addition & 1 deletion exporter/datadogexporter/traces_exporter.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ func newTracesExporter(ctx context.Context, params component.ExporterCreateSetti
params: params,
cfg: cfg,
ctx: ctx,
edgeConnection: createTraceEdgeConnection(cfg.Traces.TCPAddr.Endpoint, cfg.API.Key, params.BuildInfo),
edgeConnection: createTraceEdgeConnection(cfg.Traces.TCPAddr.Endpoint, cfg.API.Key, params.BuildInfo, cfg.TimeoutSettings),
obfuscator: obfuscator,
client: client,
denylister: denylister,
Expand Down