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

No queued retry on bad data #91

Merged

Conversation

pjanotti
Copy link
Contributor

@pjanotti pjanotti commented Jul 2, 2019

Allows the queued-retry to identify permanent errors from the consumer attached to it. This way it can safely drop batches of data data without burdening the system with retries.

@pjanotti pjanotti self-assigned this Jul 2, 2019
Paulo Janotti added 2 commits July 2, 2019 16:30
Allows the queued-retry to identify permanent errors from the consumer attached to it. This way it can safely drop batches of data data without burdening the system with retries.
@pjanotti pjanotti force-pushed the no-queued-retry-on-bad-data branch from 2d09dd1 to 33ad4ac Compare July 2, 2019 23:30
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

@codecov-io
Copy link

codecov-io commented Jul 3, 2019

Codecov Report

Merging #91 into master will decrease coverage by <.01%.
The diff coverage is 46.51%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #91      +/-   ##
==========================================
- Coverage   69.76%   69.75%   -0.01%     
==========================================
  Files         100      101       +1     
  Lines        6357     6395      +38     
==========================================
+ Hits         4435     4461      +26     
- Misses       1692     1699       +7     
- Partials      230      235       +5
Impacted Files Coverage Δ
...lector/app/sender/jaeger_thrift_tchannel_sender.go 0% <0%> (ø) ⬆️
internal/collector/processor/metrics.go 0% <0%> (ø) ⬆️
exporter/zipkinexporter/zipkin.go 72.97% <0%> (ø) ⬆️
...occollector/app/sender/jaeger_proto_grpc_sender.go 0% <0%> (ø) ⬆️
...ccollector/app/sender/jaeger_thrift_http_sender.go 0% <0%> (ø) ⬆️
...nal/collector/processor/queued/queued_processor.go 47.61% <100%> (+12.54%) ⬆️
errors/errorkind/errorkind.go 100% <100%> (ø)
exporter/opencensusexporter/opencensus.go 68.51% <0%> (-3.71%) ⬇️

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 886e62f...c03d7e7. Read the comment docs.

@tigrannajaryan
Copy link
Member

Merging #91 into master will decrease coverage by 1.67%.
The diff coverage is 46.51%.

You may want to increase test coverage, looks pretty low for this diff.

@pjanotti
Copy link
Contributor Author

pjanotti commented Jul 3, 2019

That is unexpected... not sure if it is related to update the branch or not... at https://codecov.io/gh/open-telemetry/opentelemetry-service/pull/91?src=pr&el=continue it mentions that is unable to compare... let me see what I can figure out...

@tigrannajaryan
Copy link
Member

That is unexpected... not sure if it is related to update the branch or not... at https://codecov.io/gh/open-telemetry/opentelemetry-service/pull/91?src=pr&el=continue it mentions that is unable to compare... let me see what I can figure out...

May be a codecov bug.

@pjanotti
Copy link
Contributor Author

pjanotti commented Jul 3, 2019

Build hit #59 ... updating branch...

@pjanotti pjanotti merged commit 31d6389 into open-telemetry:master Jul 3, 2019
@pjanotti pjanotti deleted the no-queued-retry-on-bad-data branch July 3, 2019 15:40
@pjanotti
Copy link
Contributor Author

pjanotti commented Jul 3, 2019

Merged due to the slowness on Travis queue. I will check the coverage numbers from other PRs.

pjanotti referenced this pull request in pjanotti/opentelemetry-service Jul 3, 2019
Previously:
    TestNewInterceptor_endToEnd

served the purpose of demoing how to make an "agent"
with the OpenCensus interceptor. It just spams stdout
with output and doesn't perform any assertions
nor serve any other purpose other than demoing.
As we add other interceptors, we'll put them in
here. Instead make it an example test that compiles
and will serve as the blueprint towards making
cmd/ocagent modifications.
@flands flands added this to the 0.1.0 milestone Jul 3, 2019
MovieStoreGuy pushed a commit to atlassian-forks/opentelemetry-collector that referenced this pull request Nov 11, 2021
* b3 propagator

* fix as per new api.

* add benchmark test.
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.

5 participants