Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

MetricLogger shouldn't throw exception #757

Closed
wants to merge 1 commit into from

Conversation

takezoe
Copy link
Member

@takezoe takezoe commented Oct 3, 2019

Fluency throws an exception when the buffer is full and fail to flush, but it shouldn't break the host logic even if such unknown errors occur inside of it.

@xerial
Copy link
Member

xerial commented Oct 3, 2019

Most likely AnyCodec has an issue if this part is throwing an exception

@xerial
Copy link
Member

xerial commented Oct 3, 2019

@takezoe So we also need to wrap TypeMetricLogger.emit

@takezoe
Copy link
Member Author

takezoe commented Oct 3, 2019

I see. I'll update to wrap emit().

@takezoe takezoe force-pushed the metric-logger-error branch from 3f26560 to 15afbcc Compare October 4, 2019 05:28
@codecov
Copy link

codecov bot commented Oct 4, 2019

Codecov Report

Merging #757 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #757      +/-   ##
==========================================
+ Coverage   83.31%   83.32%   +<.01%     
==========================================
  Files         217      217              
  Lines        8633     8634       +1     
  Branches      608      606       -2     
==========================================
+ Hits         7193     7194       +1     
  Misses       1440     1440
Impacted Files Coverage Δ
...in/scala/wvlet/airframe/fluentd/MetricLogger.scala 76.47% <100%> (+1.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 7603f16...15afbcc. Read the comment docs.

@xerial
Copy link
Member

xerial commented Oct 4, 2019

It would be great if there is an way to show warning messages at least once for the same type of errors. For example, while debugging we usually need to know the cause of errors.

@xerial
Copy link
Member

xerial commented Oct 4, 2019

Second thought:

  • Essentially we need a circuit breaker if reporting metrics becomes difficult due to fluency or network issues. This feature can be added to airframe-control.
  • Usually throwing an exception is better so that the client side program can decide what to do if metrics cannot be send. For example, we can store metrics to a disk, then resume sending metrics to fluentd if it recovers. This behavior should be configurable (e.g., report warning, ignore exceptions, throw exceptions)

@takezoe
Copy link
Member Author

takezoe commented Oct 5, 2019

Certainly, MetricLogger is not for only logging but also for sending system metrics. If sending metrics is the responsibility of the client, an exception should be handled in the client-side. OK, I withdraw this pull request and implement error handling in the client-side.

@takezoe takezoe closed this Oct 5, 2019
@takezoe takezoe deleted the metric-logger-error branch October 5, 2019 03:06
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants