Skip to content
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

Enhanced Tracing in libcurl and curl #11421

Closed
wants to merge 15 commits into from
Closed

Conversation

icing
Copy link
Contributor

@icing icing commented Jul 11, 2023

Making detailed logging availabel in libcurl and curl:

  • curl_global_log_config() for setting log configration in libcurl
  • - --trace-config command line option for curl
  • man pages
  • convert DEBUGF(LOG_CF(..)) code to CURL_LOG_CF() that is available in non-debug builds
  • protect infof() execution by verbosity check, saving some cycles when not verbose
  • test adjustments for new function

Examples:

Add HTTP/2 details to verbose output:

        curl -v --trace-config http/2 URL

Add HTTP/2, SSL and id details to verbose output:

        curl -v --trace-config http/2,ssl,ids URL

Enable all details in verbose output, including times and ids and all components in libcurl:

        curl -v --trace-config all URL

Enable all details, but not about SSL or TCP:

        curl -v --trace-config all,-ssl,-tcp URL

@github-actions github-actions bot added the tests label Jul 11, 2023
@icing icing added the logging label Jul 11, 2023
@icing icing changed the title Logging, make filter logging available in non-debug builds Enhanced Logging in libcurl and curl Aug 1, 2023
@vszakats
Copy link
Member

vszakats commented Aug 1, 2023

We'll need to update libcurl.def with the new API:

--- a/libcurl.def
+++ b/libcurl.def
@@ -28,6 +28,7 @@ curl_getenv
 curl_global_cleanup
 curl_global_init
 curl_global_init_mem
+curl_global_log_config
 curl_global_sslset
 curl_maprintf
 curl_mfprintf

@bagder
Copy link
Member

bagder commented Aug 1, 2023

We'll need to update libcurl.def with the new API:

Can't we generate that list? Or at worst make a CI job that checks that the list is up-to-date?

@vszakats
Copy link
Member

vszakats commented Aug 1, 2023

@bagder PR ready for checking it: #11570. I can't imagine a way to dynamically generate this from both CMake and Makefile.mk (and perhaps others that may be using this in the future). Converting it a raw list (without the EXPORTS header) and re-using it for more than .def purposes might work, so that a single list needs to be maintained, but it's non-trivial.

@icing
Copy link
Contributor Author

icing commented Aug 2, 2023

curl_global_log_config

Added this in the recent push. Thanks for noticing

@icing
Copy link
Contributor Author

icing commented Aug 2, 2023

Some local measurement on my macOS dev machine. I see no real difference in performance between non-debug builds on master and this PR. Meaning the guarded log statements have no impact.

curl 8.3.0-DEV (x86_64-apple-darwin22.5.0) libcurl/8.3.0-DEV OpenSSL/3.0.9 zlib/1.2.11 brotli/1.0.9 zstd/1.5.5 c-ares/1.19.1 libidn2/2.3.4 libssh2/1.11.0 nghttp2/1.55.0 ngtcp2/0.17.0 nghttp3/0.13.0 librtmp/2.3

Downloads from httpd:
> python3 tests/http/scorecard.py -d --httpd h2
              Size       Single       Serial     Parallel
master:      100MB     703 MB/s     972 MB/s     934 MB/s
this PR:     100MB     710 MB/s     986 MB/s     959 MB/s

Small requests:
> python3 tests/http/scorecard.py -r --httpd h2

Requests, max in parallel
         Size        1            6           25           50          100
master:  10KB     1665 r/s     3428 r/s     3537 r/s     3522 r/s     3475 r/s
this PR: 10KB     1676 r/s     3439 r/s     3536 r/s     3515 r/s     3227 r/s

Copy link
Member

@bagder bagder left a comment

Choose a reason for hiding this comment

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

Thought about how this can be tested, at least rudimentary?

.\" **************************************************************************
.TH curl_global_log_config 3 "01 August 2023" "libcurl" "libcurl"
.SH NAME
curl_global_log_config - Global libcurl logging configuration
Copy link
Member

@bagder bagder Aug 2, 2023

Choose a reason for hiding this comment

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

What do you think of naming it curl_global_trace() ? It is shorter and I think that is slightly more fitting name that avoids the word "log" which might have a negative connotation. We also use the word trace for this for the command line option.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, can do.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed in recent push.

The list of component names is not part of curl's public API. Names may
be added or disappear in future versions of libcurl. Since unknown names
are silently ignored, outdated log configurations will not error when
upgrading libcurl.
Copy link
Member

Choose a reason for hiding this comment

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

But we still need to document the existing available component names here, don't we? Otherwise it seems very hard to use this function!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is a trade off with this, of course. Do we want to document these names as part of the public API? Do we keep the names then in future versions?

@icing
Copy link
Contributor Author

icing commented Aug 2, 2023

Thought about how this can be tested, at least rudimentary?

Added test cases in pytest.

@icing icing changed the title Enhanced Logging in libcurl and curl Enhanced Tracing in libcurl and curl Aug 2, 2023
icing added 15 commits August 3, 2023 14:38
- WIP: app init method is missing. CURL_DEBUG env var only parsed
  on debug builds.
- add format attributes to infof() and failf() declarations
- protect `infof()` execution by verbosity check, saving some
  cycles when not verbose
- change patterns `DEBUGF(LOG_CF(...))` to `CURL_LOG_CF(...)`
- added curl_log_configure(config) to set log configuration
  explicitly
- log config extended
  - 'all' for all known components
  - +/- prefix to enable/disable
- curl_global_log_config() for setting log configration in libcurl
- --trace-config command line option for curl
- man pages
- test adjustments for new function
- curl_log.[ch] is not curl_trc.[ch]
- curl_global_log_config() is now curl_global_trace()
- CURL_LOG_CF() is now CURL_TRC_CF()
etc.
@icing
Copy link
Contributor Author

icing commented Aug 3, 2023

Ready from my side.

@bagder bagder closed this in e12b39e Aug 3, 2023
ptitSeb pushed a commit to wasix-org/curl that referenced this pull request Sep 25, 2023
Add --trace-config to curl

Add curl_global_trace() to libcurl

Closes curl#11421
vszakats added a commit to curl/curl-for-win that referenced this pull request Nov 15, 2023
- enable symbol hiding manually to match cmake/autotools.

- enable variadic macros. These macros became required in curl 8.3.0
  with curl/curl#11421.
  These macros will no longer be needed with curl 8.5.0
  after curl/curl#12167.

- set `HAVE_SNPRINTF`. curl's `config-win32.h` missed to set it,
  causing a minor difference in the generated binaries compared to
  cmake/autotools builds.
  Ref: curl/curl#12325

With these changes GNU Make builds are in sync again with
cmake/autotools.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

3 participants