[ZBXNEXT-2683] context/parameter for user macros/variables Created: 2015 Jan 27 Updated: 2016 Dec 19 Resolved: 2016 Jan 19 |
|
Status: | Closed |
Project: | ZABBIX FEATURE REQUESTS |
Component/s: | API (A), Frontend (F), Server (S) |
Affects Version/s: | 2.5.0 |
Fix Version/s: | 3.0.0alpha4 |
Type: | New Feature Request | Priority: | Trivial |
Reporter: | richlv | Assignee: | Unassigned |
Resolution: | Fixed | Votes: | 0 |
Labels: | macrocontext, macros | ||
Remaining Estimate: | Not Specified | ||
Time Spent: | Not Specified | ||
Original Estimate: | Not Specified |
Attachments: |
![]() |
||||||||||||||||
Issue Links: |
|
Description |
somewhat similar to gettext context, it would be useful to specify context for user macros/variables. context could be specified either inside the macro reference or after it :
macro resolves to the most specific value - if the context is specified, it tries to find a macro with the context value. if there is no macro with such context, it tries to find basic macro. one usecase would be lld. diskspace trigger threshold could use a macro {$DISK}. if the lld rule would specify it as :
an open question : should context or level gain priority ? as in, if host does not have macro with that context, should we :
|
Comments |
Comment by Andris Zeila [ 2015 Mar 16 ] |
Specification at http://www.zabbix.org/wiki/Docs/specs/ZBXNEXT-2683 |
Comment by Oleksii Zagorskyi [ 2015 Mar 17 ] |
I'd recreate the spec page with new issue number to avoid misunderstanding. <richlv> moved it |
Comment by Andris Zeila [ 2015 Mar 31 ] |
Server side done in development branch svn://svn.zabbix.com/branches/dev/ZBXNEXT-2683 |
Comment by Andris Zeila [ 2015 Apr 01 ] |
(1) [S] lld macro subtitution in user macro parameters will not try to quote/escape the lld macro value (the only exception is trigger expression subtitution). Because of that lld macro values startin with '"' or containing ']' will result in invalid user macro parameters. wiper The specifications changed, the context syntax is now different and the parsing was reworked. |
Comment by Andris Zeila [ 2015 Apr 10 ] |
The server side implementation was reworked according to the changed specifications in development branch svn://svn.zabbix.com/branches/dev/ZBXNEXT-2683 |
Comment by Aleksandrs Saveljevs [ 2015 Apr 13 ] |
(2) [S] Function zbx_user_macro_parse() seems to go beyond the terminating NUL if the last character in the unterminated quoted context is a backslash, e.g.: {$MACRO:"context\ It might also be useful for readability to only increase "i" for backslash only if the next character is a double quote (same as done in parse_key() function). asaveljevs That function also seems to accept macros with empty names, e.g.: {$:context} wiper RESOLVED in r53150 asaveljevs CLOSED |
Comment by Aleksandrs Saveljevs [ 2015 Apr 14 ] |
(3) [S] There seems to be a confusion with how macro names are interned, acquired, and released from the string pool. For instance, consider functions config_gmacro_add_index() and config_gmacro_remove_index(). Field "gmacro_m->macro" is released in the latter function, but is not acquired in the former. This is suspicious. One way this seems to cause a problem is in DCsync_gmacros() function: ... gmacro = DCfind_id(&config->gmacros, globalmacroid, sizeof(ZBX_DC_GMACRO), &found); /* see whether we should and can update gmacros_m index at this point */ update_index = 0; if (0 == found || 0 != strcmp(gmacro->macro, macro) || 0 != zbx_strcmp_null(gmacro->context, context)) { if (1 == found) config_gmacro_remove_index(&config->gmacros_m, gmacro); update_index = 1; } /* store new information in macro structure */ DCstrpool_replace(found, &gmacro->macro, macro); DCstrpool_replace(found, &gmacro->value, row[2]); ... Suppose global macro name has changed. Therefore, we call config_gmacro_remove_index(), which releases the macro name from the pool (the same value which "gmacro->macro" points to) and removes the index entry from the hashset. At this point, "gmacro->macro" field seems to point at an invalid value, so the behavior of DCstrpool_replace(found, &gmacro->macro, macro) is uncertain. asaveljevs Similar problem with host macros. wiper RESOLVED in r53153 asaveljevs CLOSED |
Comment by Aleksandrs Saveljevs [ 2015 Apr 14 ] |
(4) [S] Not sure whether the issue is practical, but DCsync_hmacros() function seems to have lost the ability to handle the case when a host macro is transferred unchanged to a different host. wiper Yes, it seemed that the possible gains are not worth the additional code required to 'move' the index structure to another host. asaveljevs A user macro can be transferred to a different host using "usermacro.update" method (https://www.zabbix.com/documentation/2.4/manual/api/reference/usermacro/update). REOPENED. wiper RESOLVED in r53177 asaveljevs CLOSED |
Comment by Aleksandrs Saveljevs [ 2015 Apr 14 ] |
(5) [S] Stylistically, we have a convention that a comment immediately before a line of code applies to that line of code only (or a set of consecutive non-empty lines). If a comment is meant to mark or apply to a block of code, it should be separated from the code by one line. This is mentioned at https://www.zabbix.org/wiki/C_coding_guidelines#Comments, but might not be exactly clear from the description. In this case, for example, in DCsync_hmacros() there was the following and similar changes: /* remove deleted hostmacros from buffer */ - zbx_vector_uint64_sort(&ids, ZBX_DEFAULT_UINT64_COMPARE_FUNC); zbx_hashset_iter_reset(&config->hmacros, &iter); while (NULL != (hmacro = zbx_hashset_iter_next(&iter))) { if (FAIL != zbx_vector_uint64_bsearch(&ids, hmacro->hostmacroid, ZBX_DEFAULT_UINT64_COMPARE_FUNC)) continue; config_hmacro_remove_index(&config->hmacros_hm, hmacro); zbx_strpool_release(hmacro->macro); zbx_strpool_release(hmacro->value); if (NULL != hmacro->context) zbx_strpool_release(hmacro->context); zbx_hashset_iter_remove(&iter); } This makes the comment apply to zbx_vector_uint64_sort() only, but sorting does not exactly remove anything. wiper Thanks, RESOLVED in r53154 asaveljevs CLOSED |
Comment by Aleksandrs Saveljevs [ 2015 Apr 14 ] |
(6) [S] In DCget_host_macro(), there are two loops in the beginning and a "break" in the second. I wonder whether that "break" should apply to the first loop, too. wiper Right, we should stop at the first full match rather than last. asaveljevs CLOSED |
Comment by Aleksandrs Saveljevs [ 2015 Apr 14 ] |
(7) [S] Please review stylistic fixes during code review in r53141. wiper Thanks, CLOSED |
Comment by Aleksandrs Saveljevs [ 2015 Apr 16 ] |
Server side reviewed up to r53179 and the implementation looks plausible. Will proceed with testing after frontend side is developed. |
Comment by richlv [ 2015 Jul 16 ] |
(8) compiling a list of doc updates :
iivs just because something is removed in one revision, doesn't mean it will stay that way. Macro conversion to uppercase is rewritten with JS now. <richlv> cool, thanks for the info. it sounds like the behaviour has changed still, so we might want to document that eventually |
Comment by Ivo Kurzemnieks [ 2015 Jul 21 ] |
Frontend ready for testing. |
Comment by Oleg Egorov (Inactive) [ 2015 Jul 24 ] |
(9) [F] Translation strings iivs
Added translation strings:
RESOLVED oleg.egorov CLOSED sasha updated list of changed strings @56070: Removed translation strings:
Added translation strings:
RESOLVED oleg.egorov CLOSED |
Comment by Oleg Egorov (Inactive) [ 2015 Jul 24 ] |
(10) [F] Possible save macro with lower case, enter name, then press "Enter" iivs RESOLVED in r54567 oleg.egorov CLOSED |
Comment by Aleksandrs Saveljevs [ 2015 Aug 18 ] |
(11) [F] Suppose we have the following expression in a trigger prototype in a template: {My Template:echo[{$MY.MACRO:"{#MY.CONTEXT}"}].last()} <> 0 If we attempt to change this expression by replacing "last" with "strlen", it does not allow to save the trigger with the following error: Updated: Trigger prototype "echo[{$MY.MACRO:"{#MY.CONTEXT}"}] is not empty" on "My Template". Incorrect trigger expression. Check expression part starting from "{:.}{5} <> 0". Creating such a trigger from scratch works well. iivs Tried both triggers and trigger prototypes and both create and update. I could not reproduce ths problem for now. iivs RESOLVED in r55105 asaveljevs Seems to work, but there is now a new issue on the topic, iivs I reverted the changes in r55276 and will apply the fix in |
Comment by Aleksandrs Saveljevs [ 2015 Aug 18 ] |
(12) [F] Suppose we have a macro with empty context defined globally and a macro with a quoted empty context defined on the template. These two macro are equivalent, however, the following screenshot of the host level macros does not show that: Perhaps, presentation can be a bit improved. iivs RESOLVED in r55078 asaveljevs CLOSED |
Comment by Aleksandrs Saveljevs [ 2015 Aug 18 ] |
(13) [F] Macros with context do not seem to be expanded in "Monitoring" -> "Triggers" and "Monitoring" -> "Events", at least in case there exists a macro without context, but no macro with that specific context. asaveljevs If there exists a macro without context on the trigger's host, it works. If there is only a global macro without context, it does not work. iivs RESOLVED in r55079 asaveljevs CLOSED |
Comment by Aleksandrs Saveljevs [ 2015 Aug 18 ] |
(14) [S] Suppose we have the following item: echo["{$MY.MACRO:\"comma:, bracket:] brace:} quote:\\"\"}"] It seems to be legal, but the macro does not seem to be expanded by the server. wiper RESOLVED in r55049. Note that similar problem was when trying to expand macros in item names. asaveljevs r55049 has the following diff: Index: src/libs/zbxserver/expression.c =================================================================== --- src/libs/zbxserver/expression.c (revision 55048) +++ src/libs/zbxserver/expression.c (revision 55049) @@ -389,8 +390,11 @@ while (NULL != (m = strchr(p, '$'))) { - if (m > p && '{' == *(m - 1) && NULL != (n = strchr(m + 1, '}'))) /* user defined macros */ + if (m > p && '{' == *(m - 1) && FAIL != zbx_user_macro_parse(m - 1, ¯o_r, &context_l, &context_r)) { + /* user macros */ + + n = m + context_r; c = *++n; *n = '\0'; DCget_user_macro(&hostid, 1, m - 1, &replace_to); It does not look good, because "context_r" is zero for user macros without context. Same for the other diff in substitute_simple_macros() function. Also, the comment has too many trailing spaces. REOPENED. wiper RESOLVED in r55122 asaveljevs CLOSED |
Comment by Aleksandrs Saveljevs [ 2015 Aug 18 ] |
(15) [F] User macros (with contexts) are expanded in "Configuration" -> "Hosts" in the item list in item names. They should not be, because it is a configuration section. iivs We also expand item names in popup when selecting an item for trigger expression as well as diplaying the resolved name in read-only field afterwards. This behavior is not changed since 1.8, I'm having doubts about this change. iivs Discussed with sasha and decided to leave this functionality for now and perhaps make a separate issue for it. WON'T FIX. |
Comment by Aleksandrs Saveljevs [ 2015 Aug 18 ] |
(16) [S] Create two item prototypes like these: echo[{$MY.MACRO:"{#MY.CONTEXT}"}] echo["{$MY.MACRO:\"{#MY.CONTEXT}\"}"] Send in a value for {#MY.CONTEXT} that requires quoting. Then observe the following errors in the log file: 25560:20150818:173819.533 [Z3005] query failed: [1062] Duplicate entry '10084-echo["{$MY.MACRO:\"comma:, bracket:] brace:} quote:\\"\"}"' for key 'items_1' [insert into items (itemid,name,key_,hostid,type,value_type,data_type,delay,delay_flex,history,trends,status,trapper_hosts,units,multiplier,delta,formula,logtimefmt,valuemapid,params,ipmi_sensor,snmp_community,snmp_oid,port,snmpv3_securityname,snmpv3_securitylevel,snmpv3_authprotocol,snmpv3_authpassphrase,snmpv3_privprotocol,snmpv3_privpassphrase,authtype,username,password,publickey,privatekey,description,interfaceid,flags,snmpv3_contextname) values (14,'echo "{$MY.MACRO:\\"{#MY.CONTEXT}\\"}"','echo["{$MY.MACRO:\\"comma:, bracket:] brace:} quote:\\\\"\\"}"]',10084,0,1,0,30,'',90,0,0,'','',0,0,'1','',null,'','','','','','',0,0,'',0,'',0,'','','','','',1,4,''); This may or may not have already been fixed by recent changes in wiper It was indeed fixed in RESOLVED asaveljevs CLOSED |
Comment by Andris Zeila [ 2015 Aug 19 ] |
(17) [F] It appears that $N variables in item names are not expanded to item key parameters anymore. iivs RESOLVED in r55085 asaveljevs Now they are expanded, but in a way that differs from the server. Suppose we have an item with name "echo $1" and with the following key: echo[{$MY.MACRO:}] Suppose also that {$MY.MACRO:} value is "host, plain empty context". Then, item name is shown as "echo host", which indicates that frontend parses the key for $N arguments after macro expansion, whereas server shows item name as "echo host, plain empty context", which indicates that server takes {$MY.MACRO:} as $1 and then expands macros in it (or, rather, it properly quotes the value of {$MY.MACRO:} and then parses the key to get $1). Either frontend or server should be fixed. REOPENED. asaveljevs Decided to fix this for all versions in |
Comment by Aleksandrs Saveljevs [ 2015 Aug 25 ] |
(18) [F] In host macro list, choose "Host macros". Type {$MACRO}A as macro name (note the trailing A). Choose "Inherited and host macros" and observe the following error: Undefined offset: 0 [hosts.php:953 → mergeInheritedMacros() in include/hosts.inc.php:991] iivs RESOLVED in r55232 asaveljevs CLOSED |
Comment by Aleksandrs Saveljevs [ 2015 Aug 25 ] |
(19) [F] When we are editing macros for a template, the buttons still say "Host macros" and "Inherited and host macros". Shouldn't they say "Template macros" and "Inherited and template macros" instead? asaveljevs This seems to be handled by (14) in |
Comment by Aleksandrs Saveljevs [ 2015 Aug 25 ] |
(20) [F] Suppose we are editing host macros with "Inherited and host macros" selected. In "TEMPLATE VALUE" column, click on the template - a new window will open (by the way, is it possible to make it so that the new window is opened with the template's "Macro" tab?). Now, choose "Host macros" on the host - note that "Host" tab was suddenly opened. asaveljevs There is a similar scenario to make it land on the "IPMI" tab - after you click on the template, select "Macro" tab in the template. Back to the host browser tab, choose "Host macros" - it will suddenly open "IPMI" tab. <richlv> opening the macro tab was rejected in (2) in iivs I think both of these similar scenarios are actually same issue mentioned in asaveljevs WON'T FIX |
Comment by Aleksandrs Saveljevs [ 2015 Aug 26 ] |
(21) There is a difference between frontend and server in how they handle macro parsing failures. For instance: echo[{$}{$MY.MACRO:}] Frontend sees this as echo[{$}value] while server has a different opinion: echo[{$}{$MY.MACRO:}] Should be made consistent. asaveljevs As another case, suppose we have an item with name "echo {$1}" and there is no {$1} user macro. Frontend will show "echo {arg0}", whereas server will show "echo {$1}". wiper Seems in the first case frontend is correct, in the second - server. Maybe we should split this subissue in two? wiper The first part is fixed in r55174 asaveljevs Server looks good, but there is now a different inconsistency: echo[{${$MY.MACRO:}] Frontend shows this as follows: echo[{${$MY.MACRO:}] Server, correctly, yields the following: echo[{$value] REOPENED iivs RESOLVED in r55470 asaveljevs Now the described case is OK, but it still fails with the following: echo[{$A{$MY.MACRO:}] echo[{$ABC{$MY.MACRO:}] REOPENED iivs RESOLVED in r55490 asaveljevs This time another problem was introduced. The comment for replaceUserMacros() says: * For example: * $macros[ * {$A} => {$A}, * {$B} => b, * {$C} => {$C}, * {$D} => d * ]; * * $string = "{$A}{$B}{$C}{$D}"; * * Sequence: * 1) $string = "{$A}{$B}{$C}{$D}"; // try to replace {$A}, fail, move to {$B}; * 2) $string = "{$A}b{$C}{$D}"; // try to replace {$B}, succeed, recalculate positions and restart; * 3) $string = "{$A}b{$C}{$D}"; // try to replace {$A}, fail, move to {$C}, fail, move to {$D}; * 4) $string = "{$A}b{$C}d"; // try to replace {$D}, succeed, recalculate positions, no more, exit. This algorithm is wrong, because in step 3 we should not try to replace {$A}. Indeed, suppose we have the following macros: {$A} -> a {$B} -> {$A} {$C} -> {$B} With the current implementation, all of these expand to "a". REOPENED. iivs RESOLVED in r55509 asaveljevs I took a look at user macro parsing code and have found the following cases to work incorrectly:
asaveljevs While at it, please take a look at my typo fixes in r55530. REOPENED. sasha The code was completely rewritten in r55660:55935, r55937:56070. RESOLVED oleg.egorov CLOSED |
Comment by Andris Zeila [ 2015 Sep 09 ] |
(22) [S] get_N_functionid() function manually function ids from expression. If macro context contains {<number>} it will be counted as function id, which is wrong. Note that similar problems might still be in other places that parses string that might contain user macros. wiper RESOLVED in r55513. asaveljevs In DCget_trigger_count(), is break the appropriate action here? if (NULL == (q = strchr(p + 1, '}'))) break; Maybe "goto next" is better? REOPENED wiper right, no point in continuing the parsing there. RESOLVED in r56447 asaveljevs CLOSED |
Comment by Alexander Vladishev [ 2015 Sep 11 ] |
(23) Moved from wiper RESOLVED in r55522 asaveljevs CLOSED |
Comment by Alexander Vladishev [ 2015 Sep 19 ] |
(24) [F] Broken resolving of {HOST.*} macros in the item key parameters. sasha The code was completely rewritten in r55660:55935, r55937:56070. RESOLVED oleg.egorov CLOSED |
Comment by Andris Zeila [ 2015 Oct 09 ] |
(25) [S] The macro resolving function DCget_user_macro() implementation is a bit misleading - it suggests that host base macro has more priority than global context macro, which is false. wiper RESOLVED in r56062 asaveljevs Suggested a better wording for the comment in r56463 and r56472, removed unnecessary zbx_free(macro->value). Please take a look. wiper Thanks, CLOSED |
Comment by Oleg Egorov (Inactive) [ 2015 Oct 22 ] |
(25) [F] Item/Discovery Key validation Error: Invalid key "тест" for discovery rule "macro" on "OLEG": key is empty. Key is not empty sasha RESOLVED in r56332 oleg.egorov CLOSED |
Comment by Oleg Egorov (Inactive) [ 2015 Oct 22 ] |
Under this task was improved performance in macro parsing and trigger expanding process. |
Comment by Oleg Egorov (Inactive) [ 2015 Oct 23 ] |
(26) [F] Coding style
Please review my changes in r56354
REOPENED
sasha RESOLVED in r56442 oleg.egorov CLOSED |
Comment by Ivo Kurzemnieks [ 2015 Oct 27 ] |
While rewriting the trigger expression parser there was another thing that has been automatically fixed. Before the trigger prototype dependencies for export did not work. To test if it's now working you can do the following:
oleg.egorov This issue exist in trunk, 3.0.0 aplha3, better create new ZBX and add link to sasha Please create such issue and link it. Thanks! |
Comment by Oleg Egorov (Inactive) [ 2015 Oct 30 ] |
Frontend code TESTED. |
Comment by Aleksandrs Saveljevs [ 2015 Nov 02 ] |
(27) [F] Suppose we use macro {$MY.MACRO:context}, but there is only {$MY.MACRO} defined globally. If we use {$MY.MACRO:context} in an item name, then it does not expand. asaveljevs Applied patch by sasha in r56476. RESOLVED. asaveljevs Seems to work, CLOSED. |
Comment by Aleksandrs Saveljevs [ 2015 Nov 02 ] |
Available in pre-3.0.0alpha4 (trunk) r56481. |
Comment by Andris Zeila [ 2016 Jan 08 ] |
(28) Documentation:
asaveljevs Some improvements suggested on both pages. Please take a look. We may also wish to add something to LLD macro section at https://www.zabbix.com/documentation/3.0/manual/appendix/macros/supported_by_location (mention that they can be used inside user macro contexts). wiper Thanks, looks good. I added also note about macro contexts in 'macros supported by location' as suggested. asaveljevs Perfect! CLOSED.
REOPENED wiper added https://www.zabbix.com/documentation/3.0/manual/discovery/low_level_discovery#using_lld_macros_in_user_macro_contexts sasha Perfect! CLOSED |
Comment by sergio cricca [ 2016 Feb 24 ] |
When I add this trigger: {Template OS Windows:vfs.fs.size[{#FSNAME},pfree].last(0)}<{$FS.AVERAGE:{#FSNAME}} I get this error:
Incorrect trigger expression. Check expression part starting from "}}".
Seems like it doesn't like {$MACRO:{#LLD-MACRO}} or both "}}". The only way I have, is to write it this way (without closing "}")... this seems to be accepted by frontend, but seems not to work: {Template OS Windows:vfs.fs.size[{#FSNAME},pfree].last(0)}<{$FS.AVERAGE:{#FSNAME} (trigger without context is working fine) |
Comment by Andris Zeila [ 2016 Feb 24 ] |
From documentation:
So this ought to work:
{Template OS Windows:vfs.fs.size[{#FSNAME},pfree].last(0)}<{$FS.AVERAGE:"{#FSNAME}"}
|
Comment by richlv [ 2016 Feb 24 ] |
frontend allowing to add an incorrect trigger seems to be a bug, though. |
Comment by sergio cricca [ 2016 Feb 24 ] |
created this bug report |
Comment by richlv [ 2016 Feb 24 ] |
thank you - note that you can paste only issue key, and jira will auto-detect it and link to the issue |
Comment by Alexander Vladishev [ 2016 Mar 01 ] |
richlv, why it is incorrect trigger? {Template OS Windows:vfs.fs.size[{#FSNAME},pfree].last(0)}<{$FS.AVERAGE:{#FSNAME} ^^ ^ ^ ^^ |' ' | || |'- macro -' ' cont '| '---- user macro ----' Please see wiper's comment. |
Comment by richlv [ 2016 Mar 05 ] |
ah, sorry about missing that. the quoting rule seems to be slightly confusing, but so be it |
Comment by Alexander Vladishev [ 2016 May 19 ] |
it caused a regression |
Comment by Dimitri Bellini [ 2016 Dec 16 ] |
Hi Guys, User Macro: {$NASUSEDSPACE_MIN:/vol/tech/} => 10 Trigger prototype: {Template NetApp Volume:VolSpacePercent[{#SNMPVALUE}].avg(1m)}>{$NASUSEDSPACE_MIN:"{#SNMPVALUE}"} Please pay attention on the trigger part, you have to use quoted LLD Macro ex. "{#SNMPVALUE}" but on your User Macro you have to write it (the expanded volume do you need) without ex. {$NASUSEDSPACE_MIN:/vol/tech/} Thanks so much! |
Comment by Alexander Vladishev [ 2016 Dec 17 ] |
Hi dimitri.bellini, It is already documented in User macros#Macro context section. |
Comment by Dimitri Bellini [ 2016 Dec 19 ] |
Perfect sorry, i did not see the last line of the page |