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

Jump to bottom

Handle errors in exporters #1259

Merged
merged 1 commit into from
Jul 3, 2020
Merged

Handle errors in exporters #1259

merged 1 commit into from
Jul 3, 2020

Conversation

jpkrohling
Copy link
Member

This PR changes the batch processor to not ignore the errors returned by the exporters. It also adds more context to the messages, to make it clear which exporter failed.

Signed-off-by: Juraci Paixão Kröhling [email protected]

@jpkrohling
Handle errors in exporters
de76521 
Signed-off-by: Juraci Paixão Kröhling <[email protected]>
@jpkrohling jpkrohling mentioned this pull request Jul 3, 2020
Closed
@codecov
Copy link

codecov bot commented Jul 3, 2020

Codecov Report

Merging #1259 into master will decrease coverage by 0.03%.
The diff coverage is 27.77%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1259      +/-   ##
==========================================
- Coverage   88.54%   88.50%   -0.04%     
==========================================
  Files         207      207              
  Lines       14945    14946       +1     
==========================================
- Hits        13233    13228       -5     
- Misses       1283     1286       +3     
- Partials      429      432       +3
Impacted Files Coverage Δ
exporter/opencensusexporter/opencensus.go 45.05% <0.00%> (ø)
exporter/otlpexporter/otlp.go 72.15% <33.33%> (ø)
exporter/zipkinexporter/zipkin.go 69.56% <33.33%> (ø)
processor/batchprocessor/batch_processor.go 95.77% <33.33%> (-1.39%) ⬇️
exporter/jaegerexporter/exporter.go 86.20% <50.00%> (ø)
translator/internaldata/resource_to_oc.go 86.04% <0.00%> (-4.66%) ⬇️

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 c78701f...de76521. Read the comment docs.

@bogdandrutu bogdandrutu merged commit 64403a6 into open-telemetry:master Jul 3, 2020
@vikrambe
Copy link
Contributor

vikrambe commented Jul 3, 2020

@jpkrohling @bogdandrutu I tried updating PR 1251 with these changes, PR builds are failing with the below. On local this test passes but on Build pipeline this test fails. https://app.circleci.com/pipelines/github/open-telemetry/opentelemetry-
collector/1614/workflows/5dc38e8b-5092-4a1a-88ba-448a4aff1887/jobs/10923/steps. Can you take a look ?

TestApplication_GetExporters: testing.go:906: race detected during execution of test
--- FAIL: TestApplication_GetExporters (0.01s)
: testing.go:906: race detected during execution of test
FAIL
FAIL go.opentelemetry.io/collector/service 0.366s
FAIL

@jpkrohling
Copy link
Member Author

@vikrambe are you sure it's caused by this PR here? This PR is basically just properly returning error objects to the callers. The batch processor is then logging them, but that doesn't seem to be the cause of the race condition (service/telemetry.go:110 vs. service/telemetry.go:116).

@vikrambe
Copy link
Contributor

vikrambe commented Jul 6, 2020

@jpkrohling My bad, it was due to a failing test in PR. Please ignore above comment. Thanks

wyTrivail pushed a commit to mxiamxia/opentelemetry-collector that referenced this pull request Jul 13, 2020
@jpkrohling @wyTrivail
Handle errors in exporters (open-telemetry#1259)
0811740 
Signed-off-by: Juraci Paixão Kröhling <[email protected]>
MovieStoreGuy pushed a commit to atlassian-forks/opentelemetry-collector that referenced this pull request Nov 11, 2021
@MrAlias
Add oteltest TextMap propagator and carrier (open-telemetry#1259)
8fd4b26 
* Add oteltest text map propagator and carrier

* Add changes to CHANGELOG

* Add PR number to CHANGELOG

* Add test for empty newState

Gotta farm that codecov
MovieStoreGuy pushed a commit to atlassian-forks/opentelemetry-collector that referenced this pull request Nov 11, 2021
@MrAlias
Add global propagator testing and other fixes (open-telemetry#1260)
90de306 
* Add global propagator testing and other fixes

Use the testing framework from open-telemetry#1259 to test the added global
implementation of the TextMapPropagator.

Add tests for the internal state of the global package. Ensure the
behavior of setting the global for an already set value panics.

Fix oteltest `SetN` method check on the TextMapCarrier.

* Fix lint
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bogdandrutu bogdandrutu bogdandrutu approved these changes

@dmitryax dmitryax Awaiting requested review from dmitryax

@flands flands Awaiting requested review from flands

@james-bebbington james-bebbington Awaiting requested review from james-bebbington

@owais owais Awaiting requested review from owais

@pjanotti pjanotti Awaiting requested review from pjanotti

@rghetia rghetia Awaiting requested review from rghetia

@tigrannajaryan tigrannajaryan Awaiting requested review from tigrannajaryan

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jpkrohling @vikrambe @bogdandrutu