Consider OTLP export failures handleable errors #1565
Conversation
plantfansam
requested review from
fbogsany,
mwear,
robertlaurin,
dazuma,
ericmustin,
arielvalentin,
ahayworth,
robbkidd and
kaylareopelle
as code owners
| @@ -61,6 +61,7 @@ def initialize(endpoint: nil, | |||
| @http = http_connection(@uri, ssl_verify_mode, certificate_file) | |||
|
|
|||
| @path = @uri.path | |||
| @uri_string = @uri.to_s | |||
There was a problem hiding this comment.
Doesn't feel worthwhile caching this just for use in the error message.
| log_request_failure(response.code) | ||
| FAILURE | ||
| when Net::HTTPRequestTimeOut, Net::HTTPGatewayTimeOut, Net::HTTPBadGateway | ||
| response.body # Read and discard body | ||
| redo if backoff?(retry_count: retry_count += 1, reason: response.code) | ||
| log_request_failure(response.code) |
There was a problem hiding this comment.
These two calls will double count if backoff? returns false because backoff? also does @metrics_reporter.add_to_counter('otel.otlp_exporter.failure' ...).
There was a problem hiding this comment.
Nice catch 馃槄 - will 馃 and 馃捇
Co-authored-by: Francis Bogsanyi <francis.bogsanyi@shopify.com>
Co-authored-by: Francis Bogsanyi <francis.bogsanyi@shopify.com>
|
Hey! 馃憢
Going back a bit to what you said there, we appreciate the visibility 馃挴 but at least for us it would make more sense if these failures coming from retries were logged at debug, info or warning level, since it's retried anyway (a different situation would be if it's still failing after all retry attempts). What do you think? Are you all still open to discussing it? 馃槃 |
|
馃槵 yeah, that鈥檚 bad. We shouldn鈥檛 call |
|
Sorry for the trouble @muripic, we will address this. |
|
Thank you! 鉂わ笍 |
Of course! Should be addressed by #1589 |
This PR attempts to address #1160.
The general idea is that:
handle_errormethod decide whether or not these particular failures are importantNot everyone has a metrics reporter installed, so some log-level visibility might be desirable.
Alternatively, we could
OpenTelemetry::Logger.warn(or.debugorinfowhatever) in these scenarios. I'm happy with that outcome too. Or whatever other great ideas folks may have 馃槃.