Re: [PATCH] BUILD: opentracing: excluded use of haproxy variables for, OpenTracing context
Hello Tim, On 09/11/2021 12:09 AM, Tim Düsterhus wrote: The programming style in OT is not what I'm used to, but if I understand it correctly then maybe istmatchi() is what you are searching for (it matches prefixes which should be what you are doing manually)? yes, this function could be used instead of strncasecmp(). However, it doesn't seem optimized to me and is certainly faster than strncasecmp() for simple cases for strings where the difference is right at the beginning. Already after comparing strings longer than 4 characters (and the first 4 are the same), the function is slower than strncasecmp() and the duration of the function increases rapidly while the processing time of the function strncasecmp() is quite stable). I’m not writing this to start some deeper discussion, this is purely informative. :) Best regards, -- Miroslav Zagorac Senior Developer
Re: [PATCH] BUILD: opentracing: excluded use of haproxy variables for, OpenTracing context
Hello Willy, On 09/12/2021 08:11 AM, Willy Tarreau wrote: thanks for working on fixing this, it's now merged. I've added a tiny change to make sure that text_map is always initialized in flt_ot_scope_run() because that made clang rightfully upset, re-enabled OT in the CI since it's now OK. Right, it's necessary, I overlooked it and the gcc didn't object. Thanks. -- Zaga What can change the nature of a man?
Re: [PATCH] BUILD: opentracing: excluded use of haproxy variables for, OpenTracing context
Hi guys, thanks for working on fixing this, it's now merged. I've added a tiny change to make sure that text_map is always initialized in flt_ot_scope_run() because that made clang rightfully upset, re-enabled OT in the CI since it's now OK. Cheers, Willy
Re: [PATCH] BUILD: opentracing: excluded use of haproxy variables for, OpenTracing context
Miroslav,
On 9/10/21 11:52 PM, Miroslav Zagorac wrote:
Hello Tim,
On 09/10/2021 10:55 PM, Tim Düsterhus wrote:
If it's not a NULL pointer then it must be a valid pointer. I don't
quite understand why you would need to replace this by an empty constant
string then? Is this about a missing NUL terminator? Otherwise a valid
pointer with .len == 0 would be equivalent to "".
No, v.ptr is not a null pointer and points to the wrong location, v.len
is equal to 0.
For example I added to the source two calls to the printf () function,
one before the htx_get_blk_value() function call and the other after:
-
printf(">>> n = { %p '%s' %zu }, v = { %p '%s' %zu }\n", n.ptr, n.ptr,
n.len, v.ptr, v.ptr, v.len);
v = htx_get_blk_value(htx, blk);
printf(">>> v = { %p '%s' %zu }\n", v.ptr, v.ptr, v.len);
-
I intentionally did not write the string format as '%.*s' so that the
contents can be seen. These two headers are treated here:
1, 'ot-ctx-uberctx-haproxy_id: '
2. 'ot-ctx-uber-trace-id: 3c4069d777e89019:3c4069d777e89019:0:1'
, and this is the result:
1st header:
>>> n = { 0x56079098bec1
'ot-ctx-uberctx-haproxy_idot-ctx-uber-trace-id3c4069d777e89019:3c4069d777e89019:0:1'
25 }, v = { (nil) '(null)' 0 }
>>> v = { 0x56079098beda
'ot-ctx-uber-trace-id3c4069d777e89019:3c4069d777e89019:0:1' 0 }
As can be seen from the example, v.ptr = 0x56079098beda, v.len = 0.
This looks correct to me. The value points to the location right behind
the name of the header and has length 0. The header is empty, thus the
'v' is empty as well, as indicated by v.len == 0. Thus accessing any
byte of this zero-size "buffer" is effectively undefined behavior.
2nd header:
>>> n = { 0x56079098beda
'ot-ctx-uber-trace-id3c4069d777e89019:3c4069d777e89019:0:1' 20 }, v = {
(nil) '(null)' 0 }
>>> v = { 0x56079098beee '3c4069d777e89019:3c4069d777e89019:0:1' 37 }
This also looks correct to me.
Maybe I use htx_get_blk_name()/htx_get_blk_value() in this function in
an inappropriate way, so I get such a result.
Unrelated to that: Having worked quite a bit with the ist API I would
generally recommend using the helper functions in ist.h (possibly adding
new ones if they are helpful) instead of directly touching the struct.
It makes refactoring much easier, because it's easy to find the function
calls vs. accessing struct members which might be available on different
structs.
As an example in flt_ot_http_headers_get you could use an ist for prefix
and then simply use isteqi() instead of strncasecmp().
The functions strncasecmp() and isteqi() are not compatible, because
isteqi() requires that the lengths of the strings being compared are
equal, whereas in this function that is not the case. In addition, the
prefix and len should be put in the ist structure and n.len reduced to
the size of len, this is an unnecessary complication.
You are right of course. For some reason I thought you wanted to compare
for equality and used the 'n' variant because they strings are not NUL
terminated.
The programming style in OT is not what I'm used to, but if I understand
it correctly then maybe istmatchi() is what you are searching for (it
matches prefixes which should be what you are doing manually)?
Best regards
Tim Düsterhus
Re: [PATCH] BUILD: opentracing: excluded use of haproxy variables for, OpenTracing context
Hello Tim,
On 09/10/2021 10:55 PM, Tim Düsterhus wrote:
If it's not a NULL pointer then it must be a valid pointer. I don't
quite understand why you would need to replace this by an empty constant
string then? Is this about a missing NUL terminator? Otherwise a valid
pointer with .len == 0 would be equivalent to "".
No, v.ptr is not a null pointer and points to the wrong location, v.len
is equal to 0.
For example I added to the source two calls to the printf () function,
one before the htx_get_blk_value() function call and the other after:
-
printf(">>> n = { %p '%s' %zu }, v = { %p '%s' %zu }\n", n.ptr, n.ptr,
n.len, v.ptr, v.ptr, v.len);
v = htx_get_blk_value(htx, blk);
printf(">>> v = { %p '%s' %zu }\n", v.ptr, v.ptr, v.len);
-
I intentionally did not write the string format as '%.*s' so that the
contents can be seen. These two headers are treated here:
1, 'ot-ctx-uberctx-haproxy_id: '
2. 'ot-ctx-uber-trace-id: 3c4069d777e89019:3c4069d777e89019:0:1'
, and this is the result:
1st header:
>>> n = { 0x56079098bec1
'ot-ctx-uberctx-haproxy_idot-ctx-uber-trace-id3c4069d777e89019:3c4069d777e89019:0:1'
25 }, v = { (nil) '(null)' 0 }
>>> v = { 0x56079098beda
'ot-ctx-uber-trace-id3c4069d777e89019:3c4069d777e89019:0:1' 0 }
As can be seen from the example, v.ptr = 0x56079098beda, v.len = 0.
2nd header:
>>> n = { 0x56079098beda
'ot-ctx-uber-trace-id3c4069d777e89019:3c4069d777e89019:0:1' 20 }, v = {
(nil) '(null)' 0 }
>>> v = { 0x56079098beee '3c4069d777e89019:3c4069d777e89019:0:1' 37 }
Maybe I use htx_get_blk_name()/htx_get_blk_value() in this function in
an inappropriate way, so I get such a result.
Unrelated to that: Having worked quite a bit with the ist API I would
generally recommend using the helper functions in ist.h (possibly adding
new ones if they are helpful) instead of directly touching the struct.
It makes refactoring much easier, because it's easy to find the function
calls vs. accessing struct members which might be available on different
structs.
As an example in flt_ot_http_headers_get you could use an ist for prefix
and then simply use isteqi() instead of strncasecmp().
The functions strncasecmp() and isteqi() are not compatible, because
isteqi() requires that the lengths of the strings being compared are
equal, whereas in this function that is not the case. In addition, the
prefix and len should be put in the ist structure and n.len reduced to
the size of len, this is an unnecessary complication.
> Of course, it could look like this:
>
> if (v.len == 0)
> v.ptr = "" /* or v = ist("") */;
>
In your example I would prefer v = ist("") over v.ptr = "" and leave the
heavy lifting up to the compiler for that reason.
Ok, I will use ist("").
Best regards,
--
Zaga
What can change the nature of a man?
Re: [PATCH] BUILD: opentracing: excluded use of haproxy variables for, OpenTracing context
Miroslav,
On 9/10/21 8:08 PM, Miroslav Zagorac wrote:
the problem is that in this case after calling the function
htx_get_blk_value(), v.ptr is not a NULL pointer but v.len is equal
to 0, if the http header has no value.
If it's not a NULL pointer then it must be a valid pointer. I don't
quite understand why you would need to replace this by an empty constant
string then? Is this about a missing NUL terminator? Otherwise a valid
pointer with .len == 0 would be equivalent to "".
-
Unrelated to that: Having worked quite a bit with the ist API I would
generally recommend using the helper functions in ist.h (possibly adding
new ones if they are helpful) instead of directly touching the struct.
It makes refactoring much easier, because it's easy to find the function
calls vs. accessing struct members which might be available on different
structs.
As an example in flt_ot_http_headers_get you could use an ist for prefix
and then simply use isteqi() instead of strncasecmp().
> Of course, it could look like this:
>
> if (v.len == 0)
> v.ptr = "" /* or v = ist("") */;
>
In your example I would prefer v = ist("") over v.ptr = "" and leave the
heavy lifting up to the compiler for that reason.
Best regards
Tim Düsterhus
Re: [PATCH] BUILD: opentracing: excluded use of haproxy variables for, OpenTracing context
On 09/10/2021 07:18 PM, Tim Düsterhus wrote:
Patch 2 looks odd to me. Is the issue that .ptr is equal to NULL and
*that* is causing issues? Then I recommend the following instead:
if (!isttest(v))
v = ist("");
this is much cleaner, because it does not attempt to cram so much logic
into that single huge line.
In any case if the issue is a NULL then you must test for that NULL
instead of testing the length.
Hello Tim,
the problem is that in this case after calling the function
htx_get_blk_value(), v.ptr is not a NULL pointer but v.len is equal
to 0, if the http header has no value.
Of course, it could look like this:
if (v.len == 0)
v.ptr = "" /* or v = ist("") */;
Best regards,
--
Zaga
What can change the nature of a man?
Re: [PATCH] BUILD: opentracing: excluded use of haproxy variables for, OpenTracing context
Miroslav,
On 9/10/21 12:31 PM, Miroslav Zagorac wrote:
I am sending 4 patches that revert to the opentracing filter function,
with the possibility of using haproxy variables to transfer the
opentracing context excluded.
Patch 2 looks odd to me. Is the issue that .ptr is equal to NULL and
*that* is causing issues? Then I recommend the following instead:
if (!isttest(v))
v = ist("");
this is much cleaner, because it does not attempt to cram so much logic
into that single huge line.
In any case if the issue is a NULL then you must test for that NULL
instead of testing the length.
Best regards
Tim Düsterhus
[PATCH] BUILD: opentracing: excluded use of haproxy variables for, OpenTracing context
Hello all, I am sending 4 patches that revert to the opentracing filter function, with the possibility of using haproxy variables to transfer the opentracing context excluded. Work with haproxy variables will be functional in the foreseeable future, when the necessary changes are made in the source of the opentracing filter. For a more detailed explanation of the changes you can see the description of the attached patches. Source build with OpenTracing filter support on github can be enabled. Willy, thank you for your understanding and patience. -- Zaga What can change the nature of a man? >From 77fb9a98a82c72bd0696412e5d04425d2431a951 Mon Sep 17 00:00:00 2001 From: Miroslav Zagorac Date: Wed, 8 Sep 2021 20:56:05 +0200 Subject: [PATCH 1/4] BUILD: opentracing: exclude the use of haproxy variables for the OpenTracing context Due to a recent change in the handling of haproxy variables, their use for OpenTracing context transfer has been excluded from the compilation process. The use of variables can be re-enabled if the newly defined variable OT_USE_VARS is set to 1 when calling the 'make' utility. However, this should not be used for now as the compilation will end in error. This change prevents the use of haproxy variables to convey the OpenTracing context. This means that the 'use-vars' parameter cannot be used in the OpenTracing filter configuration for 'inject' and 'extract' operations. An example configuration that uses this feature is in the test/ctx directory, while the script to run that test is test/run-ctx.sh. Then, the 'sess.ot.uuid' variable is no longer set when initializing the OpenTracing session. This means that this variable can still be used in the OpenTracing configuration, but its contents will be empty. --- Makefile | 1 + addons/ot/Makefile | 8 +++- addons/ot/include/filter.h | 2 +- addons/ot/include/parser.h | 19 +-- addons/ot/src/event.c | 6 ++ addons/ot/src/filter.c | 2 ++ addons/ot/src/parser.c | 4 addons/ot/src/scope.c | 12 ++-- 8 files changed, 48 insertions(+), 6 deletions(-) diff --git a/Makefile b/Makefile index d0a7862b6..38412275c 100644 --- a/Makefile +++ b/Makefile @@ -107,6 +107,7 @@ # OT_INC : force the include path to libopentracing-c-wrapper # OT_LIB : force the lib path to libopentracing-c-wrapper # OT_RUNPATH : add RUNPATH for libopentracing-c-wrapper to haproxy executable +# OT_USE_VARS: allows the use of variables for the OpenTracing context # IGNOREGIT : ignore GIT commit versions if set. # VERSION: force haproxy version reporting. # SUBVERS: add a sub-version (eg: platform, model, ...). diff --git a/addons/ot/Makefile b/addons/ot/Makefile index ef48ce49d..0cc8c6aad 100644 --- a/addons/ot/Makefile +++ b/addons/ot/Makefile @@ -3,6 +3,7 @@ # OT_INC : force the include path to libopentracing-c-wrapper # OT_LIB : force the lib path to libopentracing-c-wrapper # OT_RUNPATH : add libopentracing-c-wrapper RUNPATH to haproxy executable +# OT_USE_VARS : allows the use of variables for the OpenTracing context OT_DEFINE= OT_CFLAGS= @@ -61,8 +62,13 @@ OPTIONS_OBJS += \ addons/ot/src/parser.o \ addons/ot/src/pool.o\ addons/ot/src/scope.o \ - addons/ot/src/util.o\ + addons/ot/src/util.o + +ifneq ($(OT_USE_VARS),) +OT_DEFINE = -DUSE_OT_VARS +OPTIONS_OBJS += \ addons/ot/src/vars.o +endif OPTIONS_CFLAGS += $(OT_CFLAGS) -Iaddons/ot/include OPTIONS_LDFLAGS += $(OT_LDFLAGS) diff --git a/addons/ot/include/filter.h b/addons/ot/include/filter.h index 0b36354e5..c97a0cc59 100644 --- a/addons/ot/include/filter.h +++ b/addons/ot/include/filter.h @@ -22,7 +22,7 @@ #define FLT_OT_FMT_NAME "'" FLT_OT_OPT_NAME "' : " #define FLT_OT_FMT_TYPE "'filter' : " -#define FTL_OT_VAR_UUID "sess", "ot", "uuid" +#define FLT_OT_VAR_UUID "sess", "ot", "uuid" #define FLT_OT_ALERT(f, ...) ha_alert(FLT_OT_FMT_TYPE FLT_OT_FMT_NAME f "\n", ##__VA_ARGS__) #define FLT_OT_CONDITION_IF "if" diff --git a/addons/ot/include/parser.h b/addons/ot/include/parser.h index 75a39cc0c..96939916d 100644 --- a/addons/ot/include/parser.h +++ b/addons/ot/include/parser.h @@ -65,14 +65,29 @@ FLT_OT_PARSE_GROUP_DEF(ID, 0, 1, 2, 2, "ot-group", " ") \ FLT_OT_PARSE_GROUP_DEF(SCOPES, 0, 0, 2, 0, "scopes", " ...") +#ifdef USE_OT_VARS +# define FLT_OT_PARSE_SCOPE_INJECT_HELP" [use-vars] [use-headers]" +# define FLT_OT_PARSE_SCOPE_EXTRACT_HELP " [use-vars | use-headers]" +#else +# define FLT_OT_PARSE_SCOPE_INJECT_HELP" [use-headers]" +# define FLT_OT_PARSE_SCOPE_EXTRACT_HELP " [use-headers]" +#endif + +/* + * In case the possibility of working with OpenTracing context via HAProxyu + * variables is not used, args_max member of the structure flt_ot_parse_data + * should be reduced for 'inject'
