Added timeout to the exporter path #1201

jpkrohling · 2020-06-26T09:11:13Z

Signed-off-by: Juraci Paixão Kröhling juraci@kroehling.de

Description:

added context with timeout to the fan-out processor
wrapped errors recorded during the consume functions to include context about the error (where it happened, what kind of data failed to be consumed)
changed batch processor to not ignore errors, logging them as Warn instead, in line with the queued processor

Link to tracking Issue: Resolves #1193

Testing: manual tests, as per #1193

Documentation: pending

jpkrohling · 2020-06-26T09:12:39Z

This PR is to start a discussion about the right way to approach this problem. Should the timeout be configurable? If so, how?

codecov · 2020-06-26T09:27:47Z

Codecov Report

Merging #1201 into master will decrease coverage by 0.04%.
The diff coverage is 39.47%.

@@            Coverage Diff             @@
##           master    #1201      +/-   ##
==========================================
- Coverage   87.56%   87.52%   -0.05%     
==========================================
  Files         203      203              
  Lines       14555    14568      +13     
==========================================
+ Hits        12745    12750       +5     
- Misses       1370     1376       +6     
- Partials      440      442       +2

Impacted Files	Coverage Δ
exporter/opencensusexporter/opencensus.go	`45.05% <0.00%> (ø)`
exporter/otlpexporter/otlp.go	`63.24% <16.66%> (ø)`
exporter/zipkinexporter/zipkin.go	`72.54% <25.00%> (-2.97%)`	⬇️
processor/batchprocessor/batch_processor.go	`95.77% <33.33%> (-1.39%)`	⬇️
exporter/jaegerexporter/exporter.go	`86.20% <50.00%> (ø)`
processor/fanoutconnector.go	`82.50% <66.66%> (-0.36%)`	⬇️
service/service.go	`51.37% <0.00%> (-0.69%)`	⬇️
translator/internaldata/resource_to_oc.go	`73.52% <0.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4eca960...5c5a2f5. Read the comment docs.

Signed-off-by: Juraci Paixão Kröhling <juraci@kroehling.de>

pavolloffay · 2020-06-26T09:35:35Z

exporter/jaegerexporter/exporter.go

@@ -71,7 +72,7 @@ func (s *protoGRPCSender) pushTraceData(

 	batches, err := jaegertranslator.InternalTracesToJaegerProto(td)
 	if err != nil {
-		return td.SpanCount(), consumererror.Permanent(err)
+		return td.SpanCount(), consumererror.Permanent(errors.Wrap(err, "failed to push trace data via Jaeger exporter"))


IIRC more used is now fmt.Errorf within the codebase.

Looks like people prefer the Wrap, we should converge to that :)

I had fmt.Errorf but noticed that the code base seems to use errors.Wrap. I don't have a preference, to be honest :)

I think we should use fmt.Errorf with %w as we've moved to Go 1.14 and the new errors package is available now. We can actually get rid of pkg/errors package.

The nice thing about errors.Wrap is it's type safe and you don't need to add ...: %w" to every single fmt.Errorf call wrapping another error

See example https://play.golang.org/p/t9GxkVpiIhA

bogdandrutu

LGTM except the use of timeout in fanoutconnector. I think we should have this logic probably in queued_retry or in every exporter.

bogdandrutu · 2020-06-26T15:22:44Z

processor/fanoutconnector.go

@@ -25,6 +26,8 @@ import (
 	"go.opentelemetry.io/collector/internal/data"
 )

+const timeout = 5 * time.Second


The fanoutconnector is used in multiple places (between different components) and we should not have the logic of timeout here.

jpkrohling · 2020-06-29T11:36:21Z

I think we should have this logic probably in queued_retry or in every exporter.

Adding it to every exporter might still cause misbehaving exporters to block the pipeline. Adding to the queued retry could be a solution, but this processor isn't applied by default, right? I think the collector needs a protection against deadlocks/misbehaving processors/exporters.

Ideally, as someone managing a collector instance, I would probably prefer to set a deadline for the entire pipeline (or processors, or exporters), as opposed to having a timeout per exporter.

bogdandrutu · 2020-07-02T17:49:50Z

@jpkrohling let's try to make progress on this:

Split the first PR (all changes except processor/fanoutconnector.go)
Analyze the possibility to add deadline in the context - I will thing about the right solution as well.

What do you think?

jpkrohling · 2020-07-03T08:54:31Z

Done: #1259

jpkrohling · 2020-07-09T08:12:33Z

@bogdandrutu, any ideas on how to move forward with the deadline?

bogdandrutu · 2020-07-10T18:21:05Z

@jpkrohling if I add it to the queue retry is that good enough? Also currently this is not a deadlock, just a stuck operation :)

jpkrohling · 2020-07-13T13:25:30Z

if I add it to the queue retry is that good enough?

Are all users instructed to use the queued retry? If so, sure. The main idea is to cancel the execution of a pipeline component taking an unreasonable amount of time to complete.

Also currently this is not a deadlock, just a stuck operation :)

Right, I meant cases in the future where we might get affected by new bugs ;-)

bogdandrutu · 2020-07-16T22:30:27Z

@jpkrohling in #1386 I added timeout (5s) for all the exporters that use the new interface (only one that does not is zipkin).

jpkrohling · 2020-07-17T11:33:13Z

Closing in favor of #1386 .

…orter (open-telemetry#1201) Based on the spec change: open-telemetry/opentelemetry-specification#967

…y#1201) Bumps [boto3](https://github.com/boto/boto3) from 1.20.48 to 1.20.49. - [Release notes](https://github.com/boto/boto3/releases) - [Changelog](https://github.com/boto/boto3/blob/develop/CHANGELOG.rst) - [Commits](boto/boto3@1.20.48...1.20.49) --- updated-dependencies: - dependency-name: boto3 dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

jpkrohling requested review from bogdandrutu, dmitryax, flands, james-bebbington, owais, pjanotti, rghetia and tigrannajaryan as code owners June 26, 2020 09:11

jpkrohling mentioned this pull request Jun 26, 2020

OTLP Exporter blocking indefinitely #1192

Closed

Added timeout to the exporter path

5c5a2f5

Signed-off-by: Juraci Paixão Kröhling <juraci@kroehling.de>

jpkrohling force-pushed the jpkrohling/issue1193 branch from cfcc05c to 5c5a2f5 Compare June 26, 2020 09:30

pavolloffay reviewed Jun 26, 2020

View reviewed changes

bogdandrutu reviewed Jun 26, 2020

View reviewed changes

tigrannajaryan assigned bogdandrutu Jun 29, 2020

jpkrohling closed this Jul 17, 2020

Troels51 pushed a commit to Troels51/opentelemetry-collector that referenced this pull request Jul 5, 2024

fix errors in SDK documentation (open-telemetry#1201)

e1c8557

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added timeout to the exporter path #1201

Added timeout to the exporter path #1201

jpkrohling commented Jun 26, 2020 •

edited

Loading

jpkrohling commented Jun 26, 2020

codecov bot commented Jun 26, 2020 •

edited

Loading

pavolloffay Jun 26, 2020

bogdandrutu Jun 26, 2020

jpkrohling Jun 26, 2020

owais Jul 2, 2020

nilebox Jul 7, 2020 •

edited

Loading

nilebox Jul 7, 2020

bogdandrutu left a comment

bogdandrutu Jun 26, 2020

jpkrohling commented Jun 29, 2020

bogdandrutu commented Jul 2, 2020

jpkrohling commented Jul 3, 2020

jpkrohling commented Jul 9, 2020

bogdandrutu commented Jul 10, 2020

jpkrohling commented Jul 13, 2020

bogdandrutu commented Jul 16, 2020

jpkrohling commented Jul 17, 2020

Added timeout to the exporter path #1201

Added timeout to the exporter path #1201

Conversation

jpkrohling commented Jun 26, 2020 • edited Loading

jpkrohling commented Jun 26, 2020

codecov bot commented Jun 26, 2020 • edited Loading

Codecov Report

pavolloffay Jun 26, 2020

Choose a reason for hiding this comment

bogdandrutu Jun 26, 2020

Choose a reason for hiding this comment

jpkrohling Jun 26, 2020

Choose a reason for hiding this comment

owais Jul 2, 2020

Choose a reason for hiding this comment

nilebox Jul 7, 2020 • edited Loading

Choose a reason for hiding this comment

nilebox Jul 7, 2020

Choose a reason for hiding this comment

bogdandrutu left a comment

Choose a reason for hiding this comment

bogdandrutu Jun 26, 2020

Choose a reason for hiding this comment

jpkrohling commented Jun 29, 2020

bogdandrutu commented Jul 2, 2020

jpkrohling commented Jul 3, 2020

jpkrohling commented Jul 9, 2020

bogdandrutu commented Jul 10, 2020

jpkrohling commented Jul 13, 2020

bogdandrutu commented Jul 16, 2020

jpkrohling commented Jul 17, 2020

jpkrohling commented Jun 26, 2020 •

edited

Loading

codecov bot commented Jun 26, 2020 •

edited

Loading

nilebox Jul 7, 2020 •

edited

Loading