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

Fix issue where errors exporting from the collector are not logged #763

Closed
wants to merge 1 commit into from

Conversation

dashpole
Copy link
Contributor

@dashpole dashpole commented Nov 1, 2023

Fixes #761

Rather than restore the retry_on_failure component, add logging in the library directly. This has the added advantage of removing the "Try enabling retry_on_failure config option to retry on retryable errors" from the message.

@dashpole dashpole added bug Something isn't working priority: p1 labels Nov 1, 2023
@dashpole dashpole requested a review from a team as a code owner November 1, 2023 15:19
@dashpole dashpole requested a review from aabmass November 1, 2023 15:19
@dashpole dashpole force-pushed the fix_logging branch 2 times, most recently from 79ab564 to a5aa9d2 Compare November 1, 2023 15:27
Copy link

codecov bot commented Nov 1, 2023

Codecov Report

Merging #763 (63ebe18) into main (2f06d89) will decrease coverage by 0.35%.
Report is 1 commits behind head on main.
The diff coverage is 32.25%.

@@            Coverage Diff             @@
##             main     #763      +/-   ##
==========================================
- Coverage   69.65%   69.31%   -0.35%     
==========================================
  Files          42       42              
  Lines        4845     4872      +27     
==========================================
+ Hits         3375     3377       +2     
- Misses       1321     1344      +23     
- Partials      149      151       +2     
Files Coverage Δ
...er/collector/integrationtest/inmemoryocexporter.go 88.13% <100.00%> (+0.41%) ⬆️
exporter/collector/metrics.go 72.64% <25.00%> (-0.37%) ⬇️
exporter/collector/traces.go 71.92% <40.00%> (-8.08%) ⬇️
exporter/collector/logs.go 43.39% <0.00%> (-1.08%) ⬇️

... and 1 file with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Comment on lines +200 to +205
if err != nil {
l.obs.log.Error(
"Exporting logs failed",
zap.Error(err),
)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we make a generic wrapper for this kind of thing? Similar to how it worked with retry helper before

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could write a util function, but it would need access to each logger anyways. Without it being built into the component framework (e.g. like exporterhelper), I don't think we can write a generic wrapper.

@dashpole
Copy link
Contributor Author

dashpole commented Nov 2, 2023

Closing in favor of open-telemetry/opentelemetry-collector#8792

@dashpole dashpole closed this Nov 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working priority: p1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Collector exporter does not log errors when requests fail
2 participants