Skip to content

code review suggestions:#69

Merged
dgoffredo merged 1 commit intodmehala/fix-curl-issuefrom
david.goffredo/reviews/dmehala/fix-curl-issue
Nov 10, 2023
Merged

code review suggestions:#69
dgoffredo merged 1 commit intodmehala/fix-curl-issuefrom
david.goffredo/reviews/dmehala/fix-curl-issue

Conversation

@dgoffredo
Copy link
Copy Markdown
Contributor

@dgoffredo dgoffredo commented Nov 9, 2023

  • easy_setopt_timeout -> easy_setopt_timeout_ms
  • remove HTTPClient::Deadline alias
  • pass Clock into CurlImpl
    • finalize_config(const TracerConfig&, const Clock&)
    • FinalizedTracerConfig::clock
    • finalize_config(const DatadogAgentConfig&, const Clock&)
    • default_http_client(const shared_ptr<Logger>&, const Clock&)
    • FinalizedDatadogAgentConfig::clock
    • DatadogAgent::DatadogAgent
  • more specific Error::CURL_DEADLINE_EXCEEDED_BEFORE_REQUEST_START, and a little context in the error message
  • dummy_timeout -> dummy_deadline
  • TEST_CASE for Error::CURL_DEADLINE_EXCEEDED_BEFORE_REQUEST_START

@dgoffredo
code review suggestions:
8c6e74e 
- `easy_setopt_timeout`  ->  `easy_setopt_timeout_ms`
- remove `HTTPClient::Deadline` alias
- pass `Clock` into `CurlImpl`
    - `finalize_config(const TracerConfig&, const Clock&)`
    - `FinalizedTracerConfig::clock`
    - `finalize_config(const DatadogAgentConfig&, const Clock&)`
    - `default_http_client(const shared_ptr<Logger>&, const Clock&)`
    - `FinalizedDatadogAgentConfig::clock`
    - `DatadogAgent::DatadogAgent`
- more specific `Error::CURL_DEADLINE_EXCEEDED_BEFORE_REQUEST_START`,
  and a little context in the error message
- `dummy_timeout`  ->  `dummy_deadline`
- `TEST_CASE` for `Error::CURL_DEADLINE_EXCEEDED_BEFORE_REQUEST_START`
@dgoffredo dgoffredo requested a review from dmehala November 9, 2023 19:56
dmehala
dmehala reviewed Nov 10, 2023
View reviewed changes
Comment thread src/datadog/curl.cpp
Comment on lines +517 to +525
std::string message;
message +=
"Request deadline exceeded before request was even added to "
"libcurl "
"event loop. Deadline was ";
message += std::to_string(
-std::chrono::duration_cast<std::chrono::nanoseconds>(timeout)
.count());
message += " nanoseconds ago.";
Copy link
Copy Markdown
Collaborator

@dmehala dmehala Nov 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

evangelist hat May I suggest libfmt? It is header-only 😀
The fmt version would like look:

std::string message = fmt::format("Request deadline exceeded before request was even added to libcurl event loop. Deadline was {} nanoseconds ago.", std::chrono::duration_cast<std::chrono::nanoseconds>(timeout));

Much much easier to read and less error-prone right?

Copy link
Copy Markdown
Contributor Author

@dgoffredo dgoffredo Nov 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not header-only:

david@ein:~/src/fmt$ cat scratch.cpp 
#include <fmt/core.h>
#include <fmt/chrono.h>

#include <chrono>

int main() {
  const auto delay = std::chrono::microseconds(4);
  fmt::print("The delay was {}.", delay);
}

david@ein:~/src/fmt$ g++ -I include scratch.cpp
/usr/bin/ld: /tmp/ccdUMaH7.o: in function `main':
scratch.cpp:(.text+0x8e): undefined reference to `fmt::v10::vprint(fmt::v10::basic_string_view<char>, fmt::v10::basic_format_args<fmt::v10::basic_format_context<fmt::v10::appender, char> >)'
/usr/bin/ld: /tmp/ccdUMaH7.o: in function `fmt::v10::detail::error_handler::on_error(char const*)':
scratch.cpp:(.text._ZN3fmt3v106detail13error_handler8on_errorEPKc[_ZN3fmt3v106detail13error_handler8on_errorEPKc]+0xe): undefined reference to `fmt::v10::detail::throw_format_error(char const*)'
/usr/bin/ld: /tmp/ccdUMaH7.o: in function `std::make_unsigned<int>::type fmt::v10::detail::to_unsigned<int>(int)':
scratch.cpp:(.text._ZN3fmt3v106detail11to_unsignedIiEENSt13make_unsignedIT_E4typeES4_[_ZN3fmt3v106detail11to_unsignedIiEENSt13make_unsignedIT_E4typeES4_]+0x23): undefined reference to `fmt::v10::detail::assert_fail(char const*, int, char const*)'
/usr/bin/ld: /tmp/ccdUMaH7.o: in function `fmt::v10::detail::needs_escape(unsigned int)':
scratch.cpp:(.text._ZN3fmt3v106detail12needs_escapeEj[_ZN3fmt3v106detail12needs_escapeEj]+0x2d): undefined reference to `fmt::v10::detail::is_printable(unsigned int)'
/usr/bin/ld: /tmp/ccdUMaH7.o: in function `fmt::v10::detail::get_locale::get_locale(bool, fmt::v10::detail::locale_ref)':
scratch.cpp:(.text._ZN3fmt3v106detail10get_localeC2EbNS1_10locale_refE[_ZN3fmt3v106detail10get_localeC5EbNS1_10locale_refE]+0x5d): undefined reference to `std::locale fmt::v10::detail::locale_ref::get<std::locale>() const'
/usr/bin/ld: /tmp/ccdUMaH7.o: in function `std::make_unsigned<long>::type fmt::v10::detail::to_unsigned<long>(long)':
scratch.cpp:(.text._ZN3fmt3v106detail11to_unsignedIlEENSt13make_unsignedIT_E4typeES4_[_ZN3fmt3v106detail11to_unsignedIlEENSt13make_unsignedIT_E4typeES4_]+0x25): undefined reference to `fmt::v10::detail::assert_fail(char const*, int, char const*)'
/usr/bin/ld: /tmp/ccdUMaH7.o: in function `fmt::v10::detail::format_decimal_result<char*> fmt::v10::detail::format_decimal<char, unsigned int>(char*, unsigned int, int)':
scratch.cpp:(.text._ZN3fmt3v106detail14format_decimalIcjEENS1_21format_decimal_resultIPT_EES5_T0_i[_ZN3fmt3v106detail14format_decimalIcjEENS1_21format_decimal_resultIPT_EES5_T0_i]+0x43): undefined reference to `fmt::v10::detail::assert_fail(char const*, int, char const*)'
/usr/bin/ld: /tmp/ccdUMaH7.o: in function `fmt::v10::detail::format_decimal_result<char*> fmt::v10::detail::format_decimal<char, unsigned long>(char*, unsigned long, int)':
scratch.cpp:(.text._ZN3fmt3v106detail14format_decimalIcmEENS1_21format_decimal_resultIPT_EES5_T0_i[_ZN3fmt3v106detail14format_decimalIcmEENS1_21format_decimal_resultIPT_EES5_T0_i]+0x46): undefined reference to `fmt::v10::detail::assert_fail(char const*, int, char const*)'
/usr/bin/ld: /tmp/ccdUMaH7.o: in function `char const* fmt::v10::detail::parse_align<char>(char const*, char const*, fmt::v10::format_specs<char>&)':
scratch.cpp:(.text._ZN3fmt3v106detail11parse_alignIcEEPKT_S5_S5_RNS0_12format_specsIS3_EE[_ZN3fmt3v106detail11parse_alignIcEEPKT_S5_S5_RNS0_12format_specsIS3_EE]+0x4c): undefined reference to `fmt::v10::detail::assert_fail(char const*, int, char const*)'
/usr/bin/ld: scratch.cpp:(.text._ZN3fmt3v106detail11parse_alignIcEEPKT_S5_S5_RNS0_12format_specsIS3_EE[_ZN3fmt3v106detail11parse_alignIcEEPKT_S5_S5_RNS0_12format_specsIS3_EE]+0x100): undefined reference to `fmt::v10::detail::throw_format_error(char const*)'
/usr/bin/ld: /tmp/ccdUMaH7.o: in function `char const* fmt::v10::detail::parse_dynamic_spec<char>(char const*, char const*, int&, fmt::v10::detail::arg_ref<char>&, fmt::v10::basic_format_parse_context<char>&)':
scratch.cpp:(.text._ZN3fmt3v106detail18parse_dynamic_specIcEEPKT_S5_S5_RiRNS1_7arg_refIS3_EERNS0_26basic_format_parse_contextIS3_EE[_ZN3fmt3v106detail18parse_dynamic_specIcEEPKT_S5_S5_RiRNS1_7arg_refIS3_EERNS0_26basic_format_parse_contextIS3_EE]+0x71): undefined reference to `fmt::v10::detail::assert_fail(char const*, int, char const*)'
/usr/bin/ld: scratch.cpp:(.text._ZN3fmt3v106detail18parse_dynamic_specIcEEPKT_S5_S5_RiRNS1_7arg_refIS3_EERNS0_26basic_format_parse_contextIS3_EE[_ZN3fmt3v106detail18parse_dynamic_specIcEEPKT_S5_S5_RiRNS1_7arg_refIS3_EERNS0_26basic_format_parse_contextIS3_EE]+0x9a): undefined reference to `fmt::v10::detail::throw_format_error(char const*)'
/usr/bin/ld: scratch.cpp:(.text._ZN3fmt3v106detail18parse_dynamic_specIcEEPKT_S5_S5_RiRNS1_7arg_refIS3_EERNS0_26basic_format_parse_contextIS3_EE[_ZN3fmt3v106detail18parse_dynamic_specIcEEPKT_S5_S5_RiRNS1_7arg_refIS3_EERNS0_26basic_format_parse_contextIS3_EE]+0x111): undefined reference to `fmt::v10::detail::throw_format_error(char const*)'
/usr/bin/ld: /tmp/ccdUMaH7.o: in function `char const* fmt::v10::detail::parse_precision<char>(char const*, char const*, int&, fmt::v10::detail::arg_ref<char>&, fmt::v10::basic_format_parse_context<char>&)':
scratch.cpp:(.text._ZN3fmt3v106detail15parse_precisionIcEEPKT_S5_S5_RiRNS1_7arg_refIS3_EERNS0_26basic_format_parse_contextIS3_EE[_ZN3fmt3v106detail15parse_precisionIcEEPKT_S5_S5_RiRNS1_7arg_refIS3_EERNS0_26basic_format_parse_contextIS3_EE]+0x2c): undefined reference to `fmt::v10::detail::throw_format_error(char const*)'
/usr/bin/ld: /tmp/ccdUMaH7.o: in function `fmt::v10::detail::fill_t<char>::operator=(fmt::v10::basic_string_view<char>)':
scratch.cpp:(.text._ZN3fmt3v106detail6fill_tIcEaSENS0_17basic_string_viewIcEE[_ZN3fmt3v106detail6fill_tIcEaSENS0_17basic_string_viewIcEE]+0x45): undefined reference to `fmt::v10::detail::assert_fail(char const*, int, char const*)'
/usr/bin/ld: /tmp/ccdUMaH7.o: in function `int fmt::v10::detail::parse_nonnegative_int<char>(char const*&, char const*, int)':
scratch.cpp:(.text._ZN3fmt3v106detail21parse_nonnegative_intIcEEiRPKT_S5_i[_ZN3fmt3v106detail21parse_nonnegative_intIcEEiRPKT_S5_i]+0x7a): undefined reference to `fmt::v10::detail::assert_fail(char const*, int, char const*)'
/usr/bin/ld: /tmp/ccdUMaH7.o: in function `int fmt::v10::detail::to_nonnegative_int<long, int, 0>(long, int)':
scratch.cpp:(.text._ZN3fmt3v106detail18to_nonnegative_intIliLi0EEET0_T_S3_[_ZN3fmt3v106detail18to_nonnegative_intIliLi0EEET0_T_S3_]+0x55): undefined reference to `fmt::v10::detail::assert_fail(char const*, int, char const*)'
/usr/bin/ld: /tmp/ccdUMaH7.o: in function `char const* fmt::v10::detail::do_parse_arg_id<char, fmt::v10::detail::dynamic_spec_id_handler<char>&>(char const*, char const*, fmt::v10::detail::dynamic_spec_id_handler<char>&)':
scratch.cpp:(.text._ZN3fmt3v106detail15do_parse_arg_idIcRNS1_23dynamic_spec_id_handlerIcEEEEPKT_S8_S8_OT0_[_ZN3fmt3v106detail15do_parse_arg_idIcRNS1_23dynamic_spec_id_handlerIcEEEEPKT_S8_S8_OT0_]+0x8f): undefined reference to `fmt::v10::detail::throw_format_error(char const*)'
/usr/bin/ld: scratch.cpp:(.text._ZN3fmt3v106detail15do_parse_arg_idIcRNS1_23dynamic_spec_id_handlerIcEEEEPKT_S8_S8_OT0_[_ZN3fmt3v106detail15do_parse_arg_idIcRNS1_23dynamic_spec_id_handlerIcEEEEPKT_S8_S8_OT0_]+0x11d): undefined reference to `fmt::v10::detail::throw_format_error(char const*)'
/usr/bin/ld: /tmp/ccdUMaH7.o: in function `fmt::v10::detail::tm_writer<std::back_insert_iterator<fmt::v10::basic_memory_buffer<char, 500ul, std::allocator<char> > >, char, std::chrono::duration<long, std::ratio<1l, 1l> > >::tm_hour() const':
scratch.cpp:(.text._ZNK3fmt3v106detail9tm_writerISt20back_insert_iteratorINS0_19basic_memory_bufferIcLm500ESaIcEEEEcNSt6chrono8durationIlSt5ratioILl1ELl1EEEEE7tm_hourEv[_ZNK3fmt3v106detail9tm_writerISt20back_insert_iteratorINS0_19basic_memory_bufferIcLm500ESaIcEEEEcNSt6chrono8durationIlSt5ratioILl1ELl1EEEEE7tm_hourEv]+0x49): undefined reference to `fmt::v10::detail::assert_fail(char const*, int, char const*)'
/usr/bin/ld: /tmp/ccdUMaH7.o: in function `fmt::v10::detail::tm_writer<std::back_insert_iterator<fmt::v10::basic_memory_buffer<char, 500ul, std::allocator<char> > >, char, std::chrono::duration<long, std::ratio<1l, 1l> > >::tm_min() const':
scratch.cpp:(.text._ZNK3fmt3v106detail9tm_writerISt20back_insert_iteratorINS0_19basic_memory_bufferIcLm500ESaIcEEEEcNSt6chrono8durationIlSt5ratioILl1ELl1EEEEE6tm_minEv[_ZNK3fmt3v106detail9tm_writerISt20back_insert_iteratorINS0_19basic_memory_bufferIcLm500ESaIcEEEEcNSt6chrono8durationIlSt5ratioILl1ELl1EEEEE6tm_minEv]+0x49): undefined reference to `fmt::v10::detail::assert_fail(char const*, int, char const*)'
/usr/bin/ld: /tmp/ccdUMaH7.o: in function `std::make_unsigned<long long>::type fmt::v10::detail::to_unsigned<long long>(long long)':
scratch.cpp:(.text._ZN3fmt3v106detail11to_unsignedIxEENSt13make_unsignedIT_E4typeES4_[_ZN3fmt3v106detail11to_unsignedIxEENSt13make_unsignedIT_E4typeES4_]+0x25): undefined reference to `fmt::v10::detail::assert_fail(char const*, int, char const*)'
/usr/bin/ld: /tmp/ccdUMaH7.o: in function `fmt::v10::detail::tm_writer<std::back_insert_iterator<fmt::v10::basic_memory_buffer<char, 500ul, std::allocator<char> > >, char, std::chrono::duration<long, std::ratio<1l, 1l> > >::tm_sec() const':
scratch.cpp:(.text._ZNK3fmt3v106detail9tm_writerISt20back_insert_iteratorINS0_19basic_memory_bufferIcLm500ESaIcEEEEcNSt6chrono8durationIlSt5ratioILl1ELl1EEEEE6tm_secEv[_ZNK3fmt3v106detail9tm_writerISt20back_insert_iteratorINS0_19basic_memory_bufferIcLm500ESaIcEEEEcNSt6chrono8durationIlSt5ratioILl1ELl1EEEEE6tm_secEv]+0x47): undefined reference to `fmt::v10::detail::assert_fail(char const*, int, char const*)'
/usr/bin/ld: /tmp/ccdUMaH7.o: in function `fmt::v10::basic_format_parse_context<char>::next_arg_id()':
scratch.cpp:(.text._ZN3fmt3v1026basic_format_parse_contextIcE11next_arg_idEv[_ZN3fmt3v1026basic_format_parse_contextIcE11next_arg_idEv]+0x1e): undefined reference to `fmt::v10::detail::throw_format_error(char const*)'
/usr/bin/ld: /tmp/ccdUMaH7.o: in function `fmt::v10::basic_format_parse_context<char>::check_arg_id(int)':
scratch.cpp:(.text._ZN3fmt3v1026basic_format_parse_contextIcE12check_arg_idEi[_ZN3fmt3v1026basic_format_parse_contextIcE12check_arg_idEi]+0x1e): undefined reference to `fmt::v10::detail::throw_format_error(char const*)'
collect2: error: ld returned 1 exit status

And, not that this matters much, it's a lot more code:

david@ein:~/src/fmt$ g++ -I include -E scratch.cpp | wc -l
82663
david@ein:~/src/fmt$ cat nofmt.cpp 
#include <chrono>
#include <string>

extern "C" int puts(const char*);

int main() {
  const auto delay = std::chrono::microseconds(4);
  std::string message;
  message += "The delay was ";
  message += std::to_string(std::chrono::duration_cast<std::chrono::nanoseconds>(delay).count());
  message += " nanoseconds.";
  puts(message.c_str());
}

david@ein:~/src/fmt$ g++ -I include -E nofmt.cpp | wc -l
26112

std::format is in C++20, so we could use that in the next tracing library. :)

dmehala
dmehala approved these changes Nov 10, 2023
View reviewed changes
Copy link
Copy Markdown
Collaborator

@dmehala dmehala left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thank you for the new test case. :shipit:

@dgoffredo dgoffredo merged commit f52ddad into dmehala/fix-curl-issue Nov 10, 2023
@dgoffredo dgoffredo deleted the david.goffredo/reviews/dmehala/fix-curl-issue branch November 10, 2023 15:07
dmehala pushed a commit that referenced this pull request Nov 13, 2023
@dgoffredo @dmehala
code review suggestions: (#69)
4f747d9 
- `easy_setopt_timeout`  ->  `easy_setopt_timeout_ms`
- remove `HTTPClient::Deadline` alias
- pass `Clock` into `CurlImpl`
    - `finalize_config(const TracerConfig&, const Clock&)`
    - `FinalizedTracerConfig::clock`
    - `finalize_config(const DatadogAgentConfig&, const Clock&)`
    - `default_http_client(const shared_ptr<Logger>&, const Clock&)`
    - `FinalizedDatadogAgentConfig::clock`
    - `DatadogAgent::DatadogAgent`
- more specific `Error::CURL_DEADLINE_EXCEEDED_BEFORE_REQUEST_START`,
  and a little context in the error message
- `dummy_timeout`  ->  `dummy_deadline`
- `TEST_CASE` for `Error::CURL_DEADLINE_EXCEEDED_BEFORE_REQUEST_START`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dmehala dmehala dmehala approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@dgoffredo @dmehala