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

Use queue retry per exporter #2444

Merged

Conversation

pavolloffay
Copy link
Member

Signed-off-by: Pavol Loffay [email protected]

This PR configures queue retry per exporter and removes the qretry form the default configuration. This is a prefered configuration since the global qretry would retry all exporters in the pipeline. See example ocnfig from OTEL https://github.com/open-telemetry/opentelemetry-collector/blob/master/exporter/otlpexporter/config.go#L26.

Other notable changes:

  • qretry is not defined for in-memory and kafka (we will swith to OTEL kafka impl soon)

Signed-off-by: Pavol Loffay <[email protected]>
@pavolloffay
Copy link
Member Author

@joe-elliott would you like to review?

@codecov
Copy link

codecov bot commented Sep 1, 2020

Codecov Report

Merging #2444 into master will increase coverage by 0.00%.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2444   +/-   ##
=======================================
  Coverage   95.58%   95.59%           
=======================================
  Files         208      208           
  Lines       10681    10681           
=======================================
+ Hits        10209    10210    +1     
+ Misses        400      399    -1     
  Partials       72       72           
Impacted Files Coverage Δ
plugin/storage/integration/integration.go 80.86% <0.00%> (+0.47%) ⬆️

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 fc580bb...2f3d9f0. Read the comment docs.

Copy link
Member

@joe-elliott joe-elliott left a comment

Choose a reason for hiding this comment

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

Looks good to me. Two questions:

  • In several places you note // TODO: Enable the queued settings by default.. Why not just leave the defaults alone and enable it now?
  • Why not add this to the jaegerexporter? It seems like we'd want queueing and retries from agent -> collector

@pavolloffay
Copy link
Member Author

Why not add this to the jaegerexporter? It seems like we'd want queueing and retries from agent -> collector

It already supports qretry, we just wrapping it and adding support for flags.

@pavolloffay
Copy link
Member Author

In several places you note // TODO: Enable the queued settings by default.. Why not just leave the defaults alone and enable it now?

It is from https://github.com/open-telemetry/opentelemetry-collector/pull/1401/files#r457838535. I have created issue to track it open-telemetry/opentelemetry-collector#1721

I think we can enable it by default since it will be enabled anyways.

Signed-off-by: Pavol Loffay <[email protected]>
@pavolloffay pavolloffay merged commit e558711 into jaegertracing:master Sep 2, 2020
albertteoh pushed a commit to albertteoh/jaeger that referenced this pull request Sep 3, 2020
* Use queue retry per exporter

Signed-off-by: Pavol Loffay <[email protected]>

* enable qretry by default

Signed-off-by: Pavol Loffay <[email protected]>
Signed-off-by: albertteoh <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants