-
Notifications
You must be signed in to change notification settings - Fork 276
fix: Do not log error when there are no metrics to export #1953
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: Do not log error when there are no metrics to export #1953
Conversation
When collected_metrics is empty, result_code was not getting set and so report_result would fail. Reported by: @utay Related to: open-telemetry#1947 and open-telemetry#1888
| elsif result_code.nil? | ||
| OpenTelemetry.logger.debug 'No metrics to export' | ||
| else | ||
| OpenTelemetry.handle_error(exception: ExportError.new('Unable to export metrics')) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would a case statement here be better and add a debug message for the default?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was going for the simplest fix that could possibly work. To cover all the cases, I think it would be...
def report_result(result_code)
case result_code
when Export::SUCCESS
OpenTelemetry.logger.debug 'Successfully exported metrics'
when Export::FAILURE
OpenTelemetry.handle_error(exception: ExportError.new('Unable to export metrics'))
OpenTelemetry.logger.error("Result code: #{result_code}")
when Export::TIMEOUT
OpenTelemetry.handle_error(exception: ExportError.new('Export operation timed out'))
OpenTelemetry.logger.error("Result code: #{result_code}")
when nil
OpenTelemetry.logger.debug 'No metrics to export'
else
OpenTelemetry.logger.warn "Unknown result code when exporting metrics: #{result_code}"
end
end
Would you prefer that instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think so yes.
Although handle_error would be enough for the error cases since it logs by default.
So I would suggest either choosing to use handle_error or logger in those cases but not both.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that logging the result code doesn't add much value, I removed it.
|
Tested locally. Started the app, waited a bit and recorded a data point, then turned off the collector. (I am not sure how to make a timeout happen.) |
| OpenTelemetry.logger.debug 'Successfully exported metrics' | ||
| elsif result_code.nil? | ||
| when Export::FAILURE | ||
| OpenTelemetry.handle_error(exception: ExportError.new('Unable to export metrics')) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something to note here is that since this was not an exception then it will be missing a backtrace and that may confuse someone who is trying to handle the error:
irb(main):001> e = StandardError.new("bad")
=> #<StandardError: bad>
irb(main):002> e.backtrace
=> nil
The handle_error message does not need to have an exception: parameter and in this case may only need a message:.
Then again when I look at the exporter there is already a set of diagnositc errors there:
So I guess my follow up question is... why does this also need to report errors in addition to the exporters. Is there some nuance I am missing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All I really wanted was the success message. :)
I was following the pattern that Kayla set in the BLRP:
opentelemetry-ruby/logs_sdk/lib/opentelemetry/sdk/logs/export/batch_log_record_processor.rb
Lines 194 to 201 in 6f70825
| def report_result(result_code, batch) | |
| if result_code == SUCCESS | |
| OpenTelemetry.logger.debug("Successfully exported #{batch.size} log records") | |
| else | |
| OpenTelemetry.handle_error(exception: ExportError.new("Unable to export #{batch.size} log records")) | |
| OpenTelemetry.logger.error("Result code: #{result_code}") | |
| end | |
| end |
I can remove the logging for errors, and assume the thing that returned the error has already logged it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
... and it collapses down to only logging the success message. Since there is no error logging, the nil value doesn't accidentally fall into it. Problem solved!
|
Tested running locally. The only output now (again with then without a collector running) is: |
When collected_metrics is empty, result_code does not get set, and so report_result would log an error with a blank code.
Since the exporters already log if they encounter problems, there is no need to repeat the error logging here.
Reported by: @utay
Related to: #1947 and #1888