-
Notifications
You must be signed in to change notification settings - Fork 579
[OTLP/HTTP] Honor Retry-After header when retrying exports #4186
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,11 +10,14 @@ | |
|
|
||
| #include <algorithm> | ||
| #include <atomic> | ||
| #include <cctype> | ||
| #include <chrono> | ||
| #include <cmath> | ||
| #include <cstring> | ||
| #include <ctime> | ||
| #include <functional> | ||
| #include <future> | ||
| #include <iomanip> | ||
| #include <memory> | ||
| #include <random> | ||
| #include <sstream> | ||
|
|
@@ -35,6 +38,69 @@ | |
| # define CURL_VERSION_BITS(x, y, z) ((x) << 16 | (y) << 8 | (z)) | ||
| #endif | ||
|
|
||
| namespace | ||
| { | ||
| std::time_t PortableTimegm(std::tm *tm) | ||
| { | ||
| int year = tm->tm_year + 1900; | ||
| int month = tm->tm_mon + 1; | ||
|
|
||
| if (month <= 2) | ||
| { | ||
| year -= 1; | ||
| month += 12; | ||
| } | ||
|
|
||
| int day = tm->tm_mday; | ||
| int days = 365 * year + year / 4 - year / 100 + year / 400 + 367 * month / 12 - 30 + day - 719530; | ||
|
|
||
| return static_cast<std::time_t>(days) * 86400 + tm->tm_hour * 3600 + tm->tm_min * 60 + tm->tm_sec; | ||
| } | ||
|
|
||
| bool ParseRetryAfterDelay(std::string value, std::chrono::seconds &delay) | ||
| { | ||
| value.erase(0, value.find_first_not_of(" \t\r\n")); | ||
| value.erase(value.find_last_not_of(" \t\r\n") + 1); | ||
|
|
||
| if (value.empty()) | ||
| { | ||
| return false; | ||
| } | ||
|
|
||
| if (std::all_of(value.begin(), value.end(), [](unsigned char c) { return std::isdigit(c); })) | ||
| { | ||
| try | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you please use the codes just like And exception can be disabled by
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good catch. The I'll replace it with a manual digit-by-digit parsing loop that performs explicit overflow checks, following the same approach used by I'll also move the shared parsing logic into |
||
| { | ||
| delay = std::chrono::seconds(std::stoull(value)); | ||
| return true; | ||
| } | ||
| catch (...) | ||
| { | ||
| return false; | ||
| } | ||
| } | ||
| return false; | ||
| } | ||
|
|
||
| bool ParseRetryAfterDate(std::string value, std::chrono::system_clock::time_point &date) | ||
| { | ||
| value.erase(0, value.find_first_not_of(" \t\r\n")); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can use |
||
| value.erase(value.find_last_not_of(" \t\r\n") + 1); | ||
|
|
||
| std::tm tm = {}; | ||
| std::istringstream ss(value); | ||
|
|
||
| ss >> std::get_time(&tm, "%a, %d %b %Y %H:%M:%S"); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. According to https://datatracker.ietf.org/doc/html/rfc7231#section-7.1.1.1 seems support much more formats and timezone setting.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for catching this. I went back and checked §7.1.1.1 again, and you're right. The RFC defines three valid
Right now I'm only handling the first one, so I'll add support for the other two as well since the RFC requires recipients to accept all three. I'll also implement the RFC 850 two-digit year handling. If the parsed year ends up looking more than 50 years in the future, I'll map it back to the most recent matching year in the past, as required by the spec. For the timezone part, I don't think any extra handling is needed. All three formats are defined as UTC the first two explicitly use
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You're right — all valid timezones in HTTP-date are UTC/GMT, so timezone handling isn't needed. Only the datetime format variations matter. |
||
| if (!ss.fail()) | ||
| { | ||
| std::time_t epoch = PortableTimegm(&tm); | ||
|
owent marked this conversation as resolved.
|
||
| date = std::chrono::system_clock::from_time_t(epoch); | ||
| return true; | ||
| } | ||
| return false; | ||
| } | ||
| } // namespace | ||
|
|
||
| OPENTELEMETRY_BEGIN_NAMESPACE | ||
| namespace ext | ||
| { | ||
|
|
@@ -560,6 +626,11 @@ bool HttpOperation::IsRetryable() | |
|
|
||
| std::chrono::system_clock::time_point HttpOperation::NextRetryTime() | ||
| { | ||
| if (retry_after_time_point_ != std::chrono::system_clock::time_point{}) | ||
| { | ||
| return retry_after_time_point_; | ||
| } | ||
|
|
||
| static std::random_device rd; | ||
| static std::mt19937 gen(rd()); | ||
| static std::uniform_real_distribution<float> dis(0.8f, 1.2f); | ||
|
|
@@ -1463,8 +1534,9 @@ void HttpOperation::Abort() | |
| void HttpOperation::PerformCurlMessage(CURLcode code) | ||
| { | ||
| ++retry_attempts_; | ||
| last_attempt_time_ = std::chrono::system_clock::now(); | ||
| last_curl_result_ = code; | ||
| last_attempt_time_ = std::chrono::system_clock::now(); | ||
| last_curl_result_ = code; | ||
| retry_after_time_point_ = std::chrono::system_clock::time_point{}; | ||
|
|
||
| if (code != CURLE_OK) | ||
| { | ||
|
|
@@ -1513,6 +1585,29 @@ void HttpOperation::PerformCurlMessage(CURLcode code) | |
|
|
||
| if (IsRetryable()) | ||
| { | ||
| auto headers = GetResponseHeaders(); | ||
|
owent marked this conversation as resolved.
|
||
| auto it = headers.find("Retry-After"); | ||
|
|
||
| if (it != headers.end()) | ||
| { | ||
| std::string retry_after = it->second; | ||
| std::chrono::seconds delay; | ||
|
|
||
| if (ParseRetryAfterDelay(retry_after, delay)) | ||
| { | ||
| retry_after_time_point_ = std::chrono::system_clock::now() + delay; | ||
| } | ||
| else | ||
| { | ||
| std::chrono::system_clock::time_point date; | ||
| if (ParseRetryAfterDate(retry_after, date)) | ||
| { | ||
| auto now = std::chrono::system_clock::now(); | ||
| retry_after_time_point_ = (date > now) ? date : now; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Clear any response data received in previous attempt | ||
| ReleaseResponse(); | ||
| // Rewind request data so that read callback can re-transfer the payload | ||
|
|
||
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.
We can use
common::StringUtil::Trimto trim string.