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

feat(src): add new var $kong_request_id #70

Merged
merged 28 commits into from
Oct 12, 2023
Merged

Conversation

chronolaw
Copy link
Contributor

@chronolaw chronolaw commented Oct 10, 2023

See: https://konghq.atlassian.net/browse/KAG-2734

After this PR is merged, please bump the module's version.

Related PR: Kong/kong#11725

@hanshuebner
Copy link

Given that the request ID is created for every request, it may make sense to replace the use of sprintf by a specialized hexadecimal serializer. In my tests, this can reduce the time to generate the request ID string by 80%. In the grand scheme of things, this may not be too much, but it would be an improvement that is cheap to make:

#include <stdlib.h>
#include <stdio.h>

void
hex_string(uint64_t num, char *s)
{
  static const char digits[513] =
    "000102030405060708090a0b0c0d0e0f"
    "101112131415161718191a1b1c1d1e1f"
    "202122232425262728292a2b2c2d2e2f"
    "303132333435363738393a3b3c3d3e3f"
    "404142434445464748494a4b4c4d4e4f"
    "505152535455565758595a5b5c5d5e5f"
    "606162636465666768696a6b6c6d6e6f"
    "707172737475767778797a7b7c7d7e7f"
    "808182838485868788898a8b8c8d8e8f"
    "909192939495969798999a9b9c9d9e9f"
    "a0a1a2a3a4a5a6a7a8a9aaabacadaeaf"
    "b0b1b2b3b4b5b6b7b8b9babbbcbdbebf"
    "c0c1c2c3c4c5c6c7c8c9cacbcccdcecf"
    "d0d1d2d3d4d5d6d7d8d9dadbdcdddedf"
    "e0e1e2e3e4e5e6e7e8e9eaebecedeeef"
    "f0f1f2f3f4f5f6f7f8f9fafbfcfdfeff";
  uint32_t x = (uint32_t)num;
  int i = 3;
  while (i >= 0) {
    int pos = (x & 0xFF) * 2;
    char ch = digits[pos];
    s[i * 2] = ch;

    ch = digits[pos + 1];
    s[i * 2 + 1] = ch;

    x >>= 8;
    i -= 1;
  }
}


int
main()
{
  char buf[37];
  for (int i = 0; i < 10000000; i++) {
#if FAST
    hex_string(rand(), buf);
    buf[8] = 'D';
    hex_string(rand(), buf+9);
    buf[17] = 'D';
    hex_string(rand(), buf+18);
    buf[26] = 'D';
    hex_string(rand(), buf+27);
    buf[35] = 'D';
    buf[36] = 0;
#else
    sprintf(buf, "%08xD%08xD%08xD%08xD", rand(), rand(), rand(), rand());
#endif
  }

  return 0;
}

src/ngx_http_lua_kong_request_id.c Outdated Show resolved Hide resolved
src/ngx_http_lua_kong_request_id.c Outdated Show resolved Hide resolved
hanshuebner
hanshuebner previously approved these changes Oct 10, 2023
@hanshuebner hanshuebner dismissed their stale review October 10, 2023 11:57

Missing docs

Copy link

@hanshuebner hanshuebner left a comment

Choose a reason for hiding this comment

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

Please provide some documentation - Either in the pull request description or the README. The code looks good to me.

@chronolaw chronolaw marked this pull request as ready for review October 10, 2023 12:19
README.md Outdated Show resolved Hide resolved
@kikito kikito requested a review from zhongweiy October 10, 2023 16:50
@zhongweiy
Copy link

zhongweiy commented Oct 10, 2023

Have we seeded the ngx_random in other place? ngx_random calls libc random if I remember correctly and if we do not seed it, it will be seeded with 1 according to https://man7.org/linux/man-pages/man3/random.3.html.

If it has been seeded, the code looks good to me.

@chronolaw
Copy link
Contributor Author

Have we seeded the ngx_random in other place? ngx_random calls libc random if I remember correctly and if we do not seed it, it will be seeded with 1 according to https://man7.org/linux/man-pages/man3/random.3.html.

If it has been seeded, the code looks good to me.

Here it is:

os/unix/ngx_process_cycle.c:    srandom(((unsigned) ngx_pid << 16) ^ tp->sec ^ tp->msec);
os/unix/ngx_posix_init.c:    srandom(((unsigned) ngx_pid << 16) ^ tp->sec ^ tp->msec);

Copy link
Contributor

@ADD-SP ADD-SP left a comment

Choose a reason for hiding this comment

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

I think almost all changes are good except the length of $kong_request_id.

Currently, we use 8 bytes as $kong_request_id, the collision chance is sqrt(pow(256, 8)) = 4,294,967,296 (uint32_t).

My concern is about the cloud scenario, if there is a large volunm of traffic, the $kong_request_id might replicate many times in a short of period.

t/010-request-id.t Outdated Show resolved Hide resolved
@chronolaw
Copy link
Contributor Author

chronolaw commented Oct 11, 2023

I think almost all changes are good except the length of $kong_request_id.

Currently, we use 8 bytes as $kong_request_id, the collision chance is sqrt(pow(256, 8)) = 4,294,967,296 (uint32_t).

My concern is about the cloud scenario, if there is a large volunm of traffic, the $kong_request_id might replicate many times in a short of period.

Yes, it needs more discussion, Perhaps 12 bytes is a choice.

Copy link
Contributor

@ADD-SP ADD-SP left a comment

Choose a reason for hiding this comment

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

I think we can just do what the Nginx built-in $request_id did, using 16 bytes random data as $kong_request_id.

@chronolaw
Copy link
Contributor Author

chronolaw commented Oct 11, 2023

I think we can just do what the Nginx built-in $request_id did, using 16 bytes random data as $kong_request_id.

Of course, but if 8 or 12 bytes random data is enough we could get a little speed up.

src/ngx_http_lua_kong_vars.c Outdated Show resolved Hide resolved
config Outdated Show resolved Hide resolved
src/ngx_http_lua_kong_vars.c Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
t/010-request-id.t Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@dndx dndx force-pushed the feat/var_kong_request_id branch from 86cf6a7 to 242059d Compare October 12, 2023 08:54
@dndx dndx merged commit 4fbc3dd into master Oct 12, 2023
5 checks passed
@dndx dndx deleted the feat/var_kong_request_id branch October 12, 2023 09:28
samugi pushed a commit that referenced this pull request Nov 13, 2023
Similar to `$request_id`, but from pseudo-random number source instead of OpenSSL's `RAND_bytes`
for better performance.

KAG-2734
---------

Co-authored-by: Datong Sun <datong.sun@konghq.com>
samugi added a commit that referenced this pull request Nov 14, 2023
* feat(error_log): custom error handler (#65)

* Defines a new error log handler and a directive
  lua_kong_error_log_request_id that enable configuring a Request ID
  from the value of an nginx variable, to append to the error log.
* includes tests for the new directive

* tests(*): fix github workflow

Co-authored-by: Chrono <chrono_cpp@me.com>
Co-authored-by: Datong Sun <datong.sun@konghq.com>

* tests(stream): fix mockbin failing tests

* feat(variables): add new variable `$kong_request_id` (#70)

Similar to `$request_id`, but from pseudo-random number source instead of OpenSSL's `RAND_bytes`
for better performance.

KAG-2734
---------
Co-authored-by: Chrono <chrono_cpp@me.com>
Co-authored-by: Datong Sun <datong.sun@konghq.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants