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.

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

Add support for queued retry in the exporter helper. #1386

Merged
merged 2 commits into from
Jul 17, 2020

Conversation

bogdandrutu
Copy link
Member

@bogdandrutu bogdandrutu commented Jul 16, 2020

Changed only the OTLP exporter for the moment to use the new settings.

Timeout is enabled for all the exporters. Fixes #1193

There are some missing features that will be added in a followup PR:

  1. Enforcing errors. For the moment added the Throttle error as a hack to keep backwards compatibility with OTLP.
  2. Enable queued and retry for all exporters.
  3. Fix observability metrics for the case when requests are dropped because the queue is full.

@codecov
Copy link

codecov bot commented Jul 16, 2020

Codecov Report

Merging #1386 into master will increase coverage by 0.08%.
The diff coverage is 97.38%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1386      +/-   ##
==========================================
+ Coverage   90.51%   90.60%   +0.08%     
==========================================
  Files         219      220       +1     
  Lines       15516    15674     +158     
==========================================
+ Hits        14044    14201     +157     
- Misses       1059     1060       +1     
  Partials      413      413              
Impacted Files Coverage Δ
exporter/otlpexporter/otlp.go 74.73% <68.42%> (-4.40%) ⬇️
exporter/exporterhelper/queued_retry.go 98.55% <98.55%> (ø)
exporter/exporterhelper/common.go 100.00% <100.00%> (ø)
exporter/exporterhelper/logshelper.go 100.00% <100.00%> (ø)
exporter/exporterhelper/metricshelper.go 92.30% <100.00%> (+2.94%) ⬆️
exporter/exporterhelper/tracehelper.go 100.00% <100.00%> (ø)
exporter/otlpexporter/factory.go 85.50% <100.00%> (+4.73%) ⬆️

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 3274841...f7508a2. Read the comment docs.

@bogdandrutu bogdandrutu force-pushed the qretryexp branch 4 times, most recently from 69299f0 to a72309d Compare July 17, 2020 01:56
Changed only the OTLP exporter for the moment to use the new settings.

Timeout is enabled for all the exporters. Fixes open-telemetry#1193

There are some missing features that will be added in a followup PR:
1. Enforcing errors. For the moment added the Throttle error as a hack to keep backwards compatibility with OTLP.
2. Enable queued and retry for all exporters.
3. Fix observability metrics for the case when requests are dropped because the queue is full.
Copy link
Member

@jpkrohling jpkrohling left a comment

Choose a reason for hiding this comment

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

Did an initial sweep, but haven't looked at the tests yet.

General comment: would it be possible to have a generic wrapping mechanism for senders? It would make it easier to understand the chain of senders, eventually leading to the timeoutSender, which calls request.export(ctx).

exporter/exporterhelper/common.go Outdated Show resolved Hide resolved
exporter/exporterhelper/common.go Outdated Show resolved Hide resolved
exporter/exporterhelper/common.go Show resolved Hide resolved
exporter/exporterhelper/common.go Show resolved Hide resolved
exporter/exporterhelper/common.go Outdated Show resolved Hide resolved
exporter/exporterhelper/queued_retry.go Outdated Show resolved Hide resolved
exporter/exporterhelper/tracehelper.go Outdated Show resolved Hide resolved
exporter/exporterhelper/tracehelper.go Outdated Show resolved Hide resolved
exporter/otlpexporter/README.md Outdated Show resolved Hide resolved
exporter/otlpexporter/README.md Show resolved Hide resolved
Copy link
Member

@tigrannajaryan tigrannajaryan left a comment

Choose a reason for hiding this comment

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

This is a very welcome change.

exporter/exporterhelper/common.go Outdated Show resolved Hide resolved
exporter/exporterhelper/common.go Show resolved Hide resolved
exporter/exporterhelper/logshelper.go Show resolved Hide resolved
exporter/exporterhelper/queued_retry.go Outdated Show resolved Hide resolved
exporter/exporterhelper/queued_retry.go Outdated Show resolved Hide resolved
exporter/otlpexporter/README.md Outdated Show resolved Hide resolved
exporter/exporterhelper/common.go Show resolved Hide resolved
exporter/otlpexporter/README.md Outdated Show resolved Hide resolved
exporter/otlpexporter/config.go Outdated Show resolved Hide resolved
exporter/otlpexporter/README.md Outdated Show resolved Hide resolved
@bogdandrutu bogdandrutu force-pushed the qretryexp branch 3 times, most recently from 6988081 to a7ac75a Compare July 17, 2020 18:49
@bogdandrutu
Copy link
Member Author

@tigrannajaryan @jpkrohling PTAL

Thanks a lot for the review, I feel we are making progress :)

@bogdandrutu bogdandrutu force-pushed the qretryexp branch 2 times, most recently from 0828ede to 24860b5 Compare July 17, 2020 19:40
exporter/exporterhelper/common.go Outdated Show resolved Hide resolved
exporter/exporterhelper/common.go Show resolved Hide resolved
exporter/exporterhelper/queued_retry.go Outdated Show resolved Hide resolved
Copy link
Member

@tigrannajaryan tigrannajaryan left a comment

Choose a reason for hiding this comment

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

LGTM.

@bogdandrutu bogdandrutu merged commit 4046234 into open-telemetry:master Jul 17, 2020
@bogdandrutu bogdandrutu deleted the qretryexp branch July 17, 2020 21:07
hughesjj pushed a commit to hughesjj/opentelemetry-collector that referenced this pull request Apr 27, 2023
…y#1386)

Bumps [boto3](https://github.com/boto/boto3) from 1.21.23 to 1.21.24.
- [Release notes](https://github.com/boto/boto3/releases)
- [Changelog](https://github.com/boto/boto3/blob/develop/CHANGELOG.rst)
- [Commits](boto/boto3@1.21.23...1.21.24)

---
updated-dependencies:
- dependency-name: boto3
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <[email protected]>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Troels51 pushed a commit to Troels51/opentelemetry-collector that referenced this pull request Jul 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

gRPC clients need to always set a deadline/timeout
3 participants