Skip to content

Conversation

@bneradt
Copy link
Contributor

@bneradt bneradt commented May 17, 2022

Many URLs and log paths are long enough that the previous 256 byte
error.log limit for HttpSM and HttpTransact made it so that the logs got
truncated. Some of those logs had meaningful strings at the end which
made them hard to interpret unless you had the source code in front of
you to figure out what was truncated. This utilizes bwprint and local
std::string buffers for arbitrarily long log lines.

@bneradt bneradt added this to the 10.0.0 milestone May 17, 2022
@bneradt bneradt self-assigned this May 17, 2022
@bneradt
Copy link
Contributor Author

bneradt commented May 17, 2022

[approve ci autest]

@bneradt bneradt force-pushed the expand_error_log_buffer_size branch from 1d214ec to 774a121 Compare May 17, 2022 23:55
@bneradt bneradt changed the title Expand the Http* error.log buffer to 16 KB. Allow for long Http* error.log lines May 17, 2022
@bryancall
Copy link
Contributor

bryancall commented May 18, 2022

Thread local was actually slower in the benchmark I did.

Benchmark results:

----------------------------------------------------------------------------
Benchmark                                  Time             CPU   Iterations
----------------------------------------------------------------------------
thread_local_benchmark/threads:12        139 ns         1496 ns       538452
local_scope_benchmark/threads:12        6.68 ns         80.0 ns      8045064

Code:

#include <benchmark/benchmark.h>
#include <thread>

//----------------------------------------------------------------------------
static void thread_local_benchmark(benchmark::State& state) {
  for (auto _ : state) {
    thread_local std::string str;
    str.append("dlk alsdkfjsaldkfj alskdfjasldk alskdfjlasdk ldsasdkfjalsdkfj lkj falskdjfla sdjfjlsjdfl alsdjkflsfj");
    str.append("jfghkjirtyu fj alskdfjasldk alskdfjladk lasdkfjalsdkfj lkj falskdjfla sdjfjlsjdfl alsdjkflsfjdlk alsdkfjsaldkasdfasdfasd asdfasddf asdfasdf asdfasdf asdfasdfasdf afghjfghk alskdfjlasdk lasdkfjalsdkfj lkj falskdjfla sdjfjlsjdfl alsdjkflsfjdlk alsdkfjsaldkasdfasdfasd asdfasddf asdfasdf asdfasdf asdfasdfasdf afghjfghkjirtyu fj alskdfjasldk alskdfjlasdk lasdkfjalsdkfj lkj falskdjfla sdjfjlsjdfl alsdjkflsfjdfasd asdfasddf asdfas fj alskdfjasllasdk lasdkfjalsdkfj lkj falskdjfla sdjfjlsjdfl alsdjkflsfj");
  }
}

//----------------------------------------------------------------------------
static void local_scope_benchmark(benchmark::State& state) {
  for (auto _ : state) {
    std::string str;
    str.append("dlk alsdkfjsaldkfj alskdfjasldk alskdfjlasdk ldsasdkfjalsdkfj lkj falskdjfla sdjfjlsjdfl alsdjkflsfj");
    str.append("jfghkjirtyu fj alskdfjasldk alskdfjladk lasdkfjalsdkfj lkj falskdjfla sdjfjlsjdfl alsdjkflsfjdlk alsdkfjsaldkasdfasdfasd asdfasddf asdfasdf asdfasdf asdfasdfasdf afghjfghk alskdfjlasdk lasdkfjalsdkfj lkj falskdjfla sdjfjlsjdfl alsdjkflsfjdlk alsdkfjsaldkasdfasdfasd asdfasddf asdfasdf asdfasdf asdfasdfasdf afghjfghkjirtyu fj alskdfjasldk alskdfjlasdk lasdkfjalsdkfj lkj falskdjfla sdjfjlsjdfl alsdjkflsfjdfasd asdfasddf asdfas fj alskdfjasllasdk lasdkfjalsdkfj lkj falskdjfla sdjfjlsjdfl alsdjkflsfj");
  }
}


BENCHMARK(thread_local_benchmark)->Threads(12);
BENCHMARK(local_scope_benchmark)->Threads(12);
BENCHMARK_MAIN();

@bneradt
Copy link
Contributor Author

bneradt commented May 18, 2022

Thread local was actually slower in the benchmark I did.

Thank you for doing the benchmark! I'll use an undecorated std::string. Either way, this isn't hot code. Might as well keep it more simple, especially if thread_local can pessimize performance.

@bneradt bneradt force-pushed the expand_error_log_buffer_size branch from 774a121 to 5383603 Compare May 18, 2022 17:53
@bryancall
Copy link
Contributor

@cmcfarlen will review this

Many URLs and log paths are long enough that the previous 256 byte
error.log limit for HttpSM and HttpTransact made it so that the logs got
truncated. Some of those logs had meaningful strings at the end which
made them hard to interpret unless you had the source code in front of
you to figure out what was truncated. This utilizes bwprint and local
std::string buffers for arbitrarily long log lines.
@bneradt bneradt force-pushed the expand_error_log_buffer_size branch from 5383603 to 79b5089 Compare May 23, 2022 23:36
Copy link
Contributor

@cmcfarlen cmcfarlen left a comment

Choose a reason for hiding this comment

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

We talked about moving this libfmt style print into Log itself to support this directly, but this is a bit more work than intended for this PR. I'm going to make an issue to add better support for this if someone wants to work on it in the future.

Copy link
Contributor

@bryancall bryancall left a comment

Choose a reason for hiding this comment

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

Based on @cmcfarlen review

@bneradt bneradt merged commit d274b8f into apache:master Jun 7, 2022
@bneradt bneradt deleted the expand_error_log_buffer_size branch June 7, 2022 20:01
zwoop pushed a commit that referenced this pull request Jun 8, 2022
Many URLs and log paths are long enough that the previous 256 byte
error.log limit for HttpSM and HttpTransact made it so that the logs got
truncated. Some of those logs had meaningful strings at the end which
made them hard to interpret unless you had the source code in front of
you to figure out what was truncated. This utilizes bwprint and local
std::string buffers for arbitrarily long log lines.

(cherry picked from commit d274b8f)
@zwoop
Copy link
Contributor

zwoop commented Jun 8, 2022

Cherry-picked to v9.2.x

1 similar comment
@zwoop
Copy link
Contributor

zwoop commented Jun 8, 2022

Cherry-picked to v9.2.x

@zwoop zwoop modified the milestones: 10.0.0, 9.2.0 Jun 8, 2022
masaori335 pushed a commit to masaori335/trafficserver that referenced this pull request Feb 21, 2023
* asf/9.2.x:
  Updated ChangeLog
  Add proxy.process.hostdb.total_serve_stale (apache#8873)
  Allow for long Http* error.log lines (apache#8855)
  mkdfa.c is not being used and doesn't compile with gcc 12.1.1 (apache#8838)
  Add compatibility define when building with OpenSSL3 (apache#8837)
  Make post-early-return Au test more robust. (apache#8832)
  Add support for caching complete responses to the cache range requests plugin (apache#8816)
  Fixes issues with the CRR plugin introduced in apache#8488 (apache#8828)
  slice and cache_range_requests: allow header override (apache#8666) (apache#8898)
  Removed references to the throttle option from the slice plugin. (apache#8373) (apache#8897)
  cache_range_requests plugin: don't require 206 Partial Content reason string (apache#8488)
  Improve option processing in cache promote (apache#8501)
  Change parent_select Init func to constructor (apache#8853)
  Fix "is is" typos. (apache#8866)
  Eliminate duplicate words. (apache#8870)
  money_trace: allow custom header, change span-id gen, opt to create if none (apache#8655)
  Update HostDBContinuation timeout handling to clear pending queue. (apache#8480)
  Upgrade to Proxy Verifier 2.4.0. (apache#8884)
  Change ats_scoped_obj to std::unique_ptr . (apache#8882)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants