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

Tidy up consumer/consumererror package. #2768

Merged
merged 6 commits into from
Mar 23, 2021

Conversation

Aneurysm9
Copy link
Member

Description:

  • Updated docblocks for grammar and consistency
  • Remove PartialError and replace with individual signal error types
  • Rename CombineErrors to Combine
  • Refactor consumererror signal extraction to simplify exporterhelper request interface

Link to tracking Issue: #2476

Testing: Unit test suite was executed, tests were added for new and modified functions to ensure coverage. I observed an inexplicable test failure on OS X in a log exporter method where only a function invocation was changed to a new name with the same function body. It appears to be related to uber-go/zap#370 and is not reproducible on Linux via Docker.

@Aneurysm9 Aneurysm9 requested a review from a team March 23, 2021 16:12
@codecov
Copy link

codecov bot commented Mar 23, 2021

Codecov Report

Merging #2768 (ebd3673) into main (c81a01b) will decrease coverage by 0.00%.
The diff coverage is 96.82%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2768      +/-   ##
==========================================
- Coverage   91.72%   91.72%   -0.01%     
==========================================
  Files         292      292              
  Lines       15538    15555      +17     
==========================================
+ Hits        14253    14268      +15     
- Misses        880      882       +2     
  Partials      405      405              
Impacted Files Coverage Δ
consumer/consumererror/combine.go 100.00% <ø> (ø)
exporter/exporterhelper/common.go 100.00% <ø> (ø)
exporter/exporterhelper/logshelper.go 96.87% <75.00%> (-3.13%) ⬇️
exporter/exporterhelper/metricshelper.go 97.14% <75.00%> (-2.86%) ⬇️
config/configcheck/configcheck.go 100.00% <100.00%> (ø)
consumer/consumererror/permanent.go 100.00% <100.00%> (ø)
consumer/consumererror/signalerrors.go 100.00% <100.00%> (ø)
consumer/fanoutconsumer/cloningconsumer.go 57.57% <100.00%> (ø)
consumer/fanoutconsumer/consumer.go 100.00% <100.00%> (ø)
exporter/exporterhelper/queued_retry.go 97.70% <100.00%> (-0.02%) ⬇️
... and 14 more

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 c81a01b...ebd3673. Read the comment docs.

consumer/consumererror/signalerrors.go Outdated Show resolved Hide resolved
consumer/consumererror/signalerrors.go Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
consumer/consumererror/signalerrors.go Outdated Show resolved Hide resolved
consumer/consumererror/signalerrors.go Outdated Show resolved Hide resolved
consumer/consumererror/signalerrors.go Outdated Show resolved Hide resolved
consumer/consumererror/signalerrors.go Outdated Show resolved Hide resolved
exporter/exporterhelper/common.go Outdated Show resolved Hide resolved
* Updated docblocks for grammar and consistency
* Added `IsPartial()` predicate to match `IsPermanent()`
* Ensured tests for `PartialError` test the public interface

Remove `PartialError` and replace with individual signal error types

Refactor consumererror signal extraction to simplify exporterhelper request interface
This moves the accessors for signal data to methods on the individual error types
and provides As<Signal>() package functions that behave as targeted versions of
the errors.As() function.
@Aneurysm9 Aneurysm9 force-pushed the consumererror_tidy branch from 76ccbe6 to 2a2aa5e Compare March 23, 2021 20:33
CHANGELOG.md Outdated Show resolved Hide resolved
consumer/consumererror/combine.go Outdated Show resolved Hide resolved
exporter/exporterhelper/logshelper.go Outdated Show resolved Hide resolved
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.

2 participants