seems to be a duplicate of ZBXNEXT-1836 / ZBXNEXT-714 ?

Not exactly. ZBXNEXT-714 asks for Zabbix database replacement with other storage for history tables. This issue is more about duplicating history elsewhere. I'll update the description to make it clear.

it seems that pluggable storage modules could allow more than one storage at a time, solving both of these similar needs - but if they are different enough to warrant a separate issue, all is good

I would say it is a smaller and easier step in the direction of ZBXNEXT-714. We and module developers will not need to implement both server-to-module-to-storage and storage-to-module-to-server data transfer, just the former. And for implementing ZBXNEXT-714 now we would need either frontend plugin support or server-side API...

makes a lot of sense - thanks for the explanation

To not affect server performance, this should ideally be decoupled from history synchronization. Ad-hoc I see two approaches:

1. A shadow history cache.
2. A queuing mechanism.

Well, one could also argue for simplicity reasons to put it into the already overloaded chain of history synchronization and let the loadable module decide whether to cache, queue, or whatsoever to do with it.

It is not possible not to affect server performance. No matter how it is implemented it will be one more thing for server to do. If reasonable scalability is ensured it would be enough for now. Keeping in mind ZBX-10428 it's more important to provide well-thought-through API for maximum forward- and easily maintainable backward-compatibility. If this goal is reached, it will allow to optimize server implementation later on without disrupting functionality of modules which will have been written by that moment in the future.

"Not affecting server performance" was rather meant in the sense of not blocking history synchronization.
While being still convinced that this can be avoided by the mentioned and possibly other approaches, it's likely not possible without investing significant effort in the design and implementation.

I just assumed the possibly most "simple" and sub-optimal implementation, that's to say, to call a function for each and every value to synchronize. While writing that, I realize that "letting the loadable module decide" is actually not a bad idea.

However, I'm not the expert and looking forward to read the specification....

Ready for testing in development branch svn://svn.zabbix.com/branches/dev/ZBXNEXT-3353 revision 61520.

(1) We call zbx_sync_history_with_modules() in DCsync_history() outside cache locks. By default StartDBSyncers=4.

This implies that the module writers might have to create their own locking mechanism in order to:

• prevent data corruption when writing to files, logs, etc.;
• preserve the same order as in Zabbix DB (is this important?).

Possible solutions:

• allow module writers to optionally run their code inside cache locks;
• provide more complete 'plugin' mechanism similar to interrupt handler model in kernel:
  • do the absolute minimum work in the 'interrupt handler';
  • do all the remaining work in another thread of execution (could be a separate process in our case);

Regardless of what we chose here we should at least provide some hints in the docs.

glebs.ivanovskis Solving this on Zabbix side will produce unnecessary overhead for users who want to write KISS module and whose external storage can handle parallel input. Asked sasha about this and letting modules inside cache locks is absolutely not an option. I believe we should provide an example of synchronization in module developer documentation.

Currently in dummy module we call srand() in zbx_module_init() which is called by Zabbix main process before forking. This means that all poller processes will give out the same sequence of pseudo random numbers when polling dummy.random[]. Sorting this issue can be a good ground for an example.

glebs.ivanovskis Worth a separate ZBX-11811.
CLOSED

(2) [D] We should be explicit on what is and is not supported in modules:

• calling Zabbix internal functions (DB API, logging, locking, data structures, memory mgmt.), using macros except the ones specifically mentioned in module API;
• using Zabbix locks and/or attaching to shared memory segments used by Zabbix;
• linking against other libraries that might be used by Zabbix itself;
• loading and unloading other modules on behalf of Zabbix;
• exporting fake history data through other modules on behalf of Zabbix ( glebs.ivanovskis);
• compiler ABI requirements (does the module have to be compiled with the same compiler, release of the compiler as Zabbix);
• security considerations - running 3rd party code directly in Zabbix for which there might be exception policies set in SELinux, etc.
• accessing, modifying global variables, objects;
• accessing, modifying Zabbix DB;
• overriding signal handlers;
• subset of standard C, POSIX functions and syscalls (exit() is a clear example, but there are others);
• naming conventions;
• etc.

Our documentation (https://www.zabbix.com/documentation/3.2/manual/config/items/loadablemodules) suggests including log.h and using zabbix_log() however, after discussing with alexei, it seems that this was not the original intention.

glebs.ivanovskis Indeed, note about zabbix_log() was added later. But still a long time ago. And we can't deny that modules can call functions from Zabbix binary and access variables with extern visibility if Zabbix was compiled with -rdynamic option. In this respect libcurl documentation says the following:

All public functions in the libcurl interface are prefixed with 'curl_' (with a lowercase c). You can find other functions in the library source code, but other prefixes indicate that the functions are private and may change without further notice in the next release.

Only use documented functions and functionality!

Knowing that any function/variable with extern visibility can be reached by "strangers" should keep us fit and make us pay more attention to our codebase structure.

glebs.ivanovskis dlmopen() provides an option to load up to 15 modules into their own "namespaces" and cherry-pick which functions we allow them to use, but dlmopen() is a GNU extension and won't be portable.

glebs.ivanovskis Offloaded this tough one to ZBX-11813.
CLOSED

(3) Store value setting is not respected.

RESOLVED in r61604.

sandis.neilands CLOSED.

(4) [D] Please describe module data export mechanism in the documentation.

glebs.ivanovskis Added a few words here.

VSI Not much to check there about texts. I would suggest this part to be checked by a person fluent with code and confirm, that all the main points of the solution are described.

wiper CLOSED

(5) [D] Let's agree and document our policy regarding modules:

1. module API stability guarantees - one LTS release only?;
2. Zabbix API guarantees - using anything from Zabbix internals is not officially supported?;
3. binary compatibility guarantees - user's responsibility:
   1. Zabbix SIA is not the only source of the binaries;
   2. libraries used by Zabbix are usually precompiled by third parties;
   3. support for many features depends on the build options;
   4. third party extensions, modifications to Zabbix source code;
   5. other third-party modules;
   6. OS, architecture differences between installations;
   7. different compilers with various options can lead to incompatibility;
   8. different versions of the same compiler can lead to incompatibility.
4. licensing issues for modules distributed in binary form - does the module have to be GPL? - or is it better to leave this ambiguous as it is with the LKMs?

glebs.ivanovskis Dear alexei and sasha, please have a look and provide official answers. I will then proceed with documentation.

glebs.ivanovskis Answers from alexei (please make sure that I got you right):

1. module API stability is guaranteed during one LTS release cycle;
2. stability of Zabbix API is not guaranteed (technically it is possible to call Zabbix internal functions from module, but there is no warranty whatsoever that such modules will work);
3. binary compatibility is not guaranteed (this implies that distribution of modules in binary form has no grounding), sandis.neilands brilliantly described the reasons;
4. this aspect is governed by GPL license (modules are linking with Zabbix in runtime and are using Zabbix headers, currently whole Zabbix code is licensed as GPL).

martins-v Added to the loadable module section for 3.4 (last two paragraphs in 'Overview'). Please review before it could be copied to other doc versions.

glebs.ivanovskis Looks fine to me. Added a couple of words to cover (14) too. As we don't provide binary compatibility, strictly speaking, module recompilation is a part of upgrade procedure, but it's not super-critical for the upgrade itself.
CLOSED

Attached a prototype with an alternative design for exporting history data in real time. Example module for exporting string data to CSV file also included.

The patch is for trunk circa 3.2alpha2.

Design principles:

• orthogonal API - 'same' history objects as in Zabbix API;
• preserve export locality - export when Zabbix saves data itself;
• lightweight module infrastructure - no dynamic memory allocations, no parsing of strings, not even 'if' statements to keep the CPU pipe-lining in tact.

E.g. the main idea is - get the data out as fast as possible and give control back to Zabbix. Then do any post processing, etc outside of Zabbix address space.

Implications:

• module history export API would be changed together (if ever) with Zabbix history.get API;
• familiar API - low learning curve;
• convenient - history export done in lock context (same as Zabbix itself) - no need for external locking to guarantee order and consistency;
• saving of history data to Zabbix database can be reimplemented as loadable module and handled together with other data saving modules.

(6) [S] [P] Please add header comments for external API functions zbx_next_history_index(), zbx_get_history_field(), zbx_get_history_type().

glebs.ivanovskis RESOLVED in r61909.

sandis.neilands CLOSED.

(7) [S] [P] Add data-export module template or a simple module that saves to file, syslog or other POSIX interface that doesn't require external dependencies.

sandis.neilands CLOSED. Dummy module contains placeholder code for history data export.

(8) [S] [P] Compiler warnings with clang.

../../../include/module.h:77:14: warning: 'const' type qualifier on return type has no effect [-Wignored-qualifiers]
const char * const      zabbix_error_message(zabbix_error_t error);
             ^~~~~

../../../include/module.h:77:14: warning: 'const' type qualifier on return type has no effect [-Wignored-qualifiers]
const char * const      zabbix_error_message(zabbix_error_t error);
             ^~~~~

glebs.ivanovskis Nice catch! RESOLVED in r61914. Please have a look at cosmetic changes in r61916, r61923 and r61924 too.

sandis.neilands CLOSED.

(9) [S] [P] Optional: there is now co-dependency of zbxmodules.h and dbcache mostly due to zbx_sync_history_with_modules() "glue" function not belonging in either of them.

glebs.ivanovskis Well, you have to draw the line somewhere. To my understanding, everything with "modules" in it's name belongs to zbxmodules lib, plus modules.c is not that huge as dbcache.c. Idea was to keep loaded module array isolated in modules.c and history structure definitions isolated in dbcache.c, and to some extent it was achieved. Although we currently have zbx_sync_history_with_modules(), zbx_next_history_index(), zbx_get_history_field(), zbx_get_history_type() exposed in headers they are useless without a proper history array pointer which is hidden in dbcache.c.

WON'T FIX, I guess.

(10) [S] [P] Optional: please invent more specific type names than zabbix_error_t, zabbix_label_t, zabbix_handle_t. This also applies to names of the #defines.

glebs.ivanovskis Rationale behind generic naming was that the same interface (zabbix_get_object_member() and zabbix_get_vector_element() functions, aforementioned types and a bunch of #defines) can be used in the future to export various other things, not just history. Another thing is that these names will be used by module writers, let's break our tradition and provide a nice-from-user-perspective solution. Using zabbix_ prefix versus usual zbx_ is enough to make a distinction (please remind me to add this to guidelines if I forget).

WON'T FIX

Just for the history I'm attaching the abandoned implementation patch.

$ svn diff ^/branches/dev/ZBXNEXT-3353@61401 ^/branches/dev/ZBXNEXT-3353@61924

Ready for testing in development branch svn://svn.zabbix.com/branches/dev/ZBXNEXT-3353 revision 62033.

(11) [S] [P] [G] Current implementation skips those modules that do not export ZBX_MODULE_FUNC_ITEM_LIST function. Modules that only have the history export part will not be loaded.

glebs.ivanovskis Out of scope of this development. There are ZBX-8222, ZBX-11058 and ZBXNEXT-2666 to change the situation. Workaround is quite simple:

static ZBX_METRIC keys[] = {{NULL}};

ZBX_METRIC	*zbx_module_item_list()
{
	return keys;
}

WON'T FIX

sandis.neilands After discussing with sasha it was decided to make all module functions optional except zbx_module_api_version(). REOPENED.

glebs.ivanovskis RESOLVED in r62236.

sasha CLOSED

(12) [S] [P] In DCmass_add_history() and dc_add_history_*() we skip values with h->keep_history == 0 however we export these values to modules. It would be better to refactor all incarnations of the following code in separate function.

                if (0 != (ZBX_DC_FLAG_UNDEF & h->flags) || 0 != (ZBX_DC_FLAG_NOVALUE & h->flags))
                        continue;

                if (0 == h->keep_history)
                        continue;

Also please add a comment describing why it was decided not to export LLD values.

sandis.neilands After discussing with sasha and alexei it was decided to not export values with h->keep_history == 0.

glebs.ivanovskis Please have a look at r62215 and say if it is what you wanted.
LLD values do not go into Zabbix own DB, only into proxy DB and only for temporary storage. If user wants to see actual LLD data he/she can create regular items with discovery keys. Then such LLD data will be exported to modules too. I can't imagine a use of LLD data outside of Zabbix. And the last but not the least, LLD data on server has virtually no size limit (Only 128 MiB limit in zbx_tcp_recv_ext() I guess) and probably is too bulky.
RESOLVED

sandis.neilands Very nice. CLOSED.

(13) [S] [P] For ITEM_VALUE_TYPE_LOG - should we set h_log->source to "" as is done in dc_add_history_log() when h->value.str == NULL?

glebs.ivanovskis RESOLVED in r62165 and r62174, threw a couple of const here and there in r62166, please review.

sandis.neilands CLOSED.

Some performance testing (while Zabbix does not write items with keep_history = 0 to its own DB but exports them through modules):

1. new solution with actual export;
2. abandoned solution with actual export;
3. new solution with actual export (again);
(* External storage is doing something in background, I suppose.)
4. new solution with empty export (i.e. it's registering export capabilities but does nothing);
5. abandoned solution with empty export.

I can't see any significant difference.

(14) [D] Statement regarding module licensing is needed.

glebs.ivanovskis Like 4. from (5), this is governed by GPL terms.

glebs.ivanovskis RESOLVED together with (5).
CLOSED

(15) [D] Although export from proxies is allowed (for distributed solutions) data exported from proxies will be different from data exported from server:

• custom multipliers won't be applied;
• ...

glebs.ivanovskis Added a note here.

VSI Looks OK to me.

wiper Delta item calculation is also done by server - only raw values will be available on proxy.

And starting with 3.4 the data type conversions (boolean/ocatal/hexadecimal to decimal) are done by server.

glebs.ivanovskis Good point, thank you! Updated note, tried to make it vague and future-proof:

• https://www.zabbix.com/documentation/3.2/manual/config/items/loadablemodules#providing_history_export_callbacks
• https://www.zabbix.com/documentation/3.4/manual/config/items/loadablemodules#providing_history_export_callbacks

If we need to add more details to the latter, let's move it to a subissue of 3.4 feature. (ZBXNEXT-1443?)

wiper Looks good. For 3.4 we could probably simply mention that item preprocessing is done on server and proxy history will contain only raw values.
CLOSED

Available in pre-3.2.0rc1 (trunk) r62323.

(16) [A] [P] [S] Daemons do not start without modules.

glebs.ivanovskis RESOLVED in r62348.

sasha CLOSED

(17) [S] Server tries to keep trends for non-numeric items.

glebs.ivanovskis RESOLVED in r62348.

sasha CLOSED

Fixes for subissues (16) and (17) are available in new development branch svn://svn.zabbix.com/branches/dev/ZBXNEXT-3353

Subissues (16) and (17) are fixed in pre-3.2.0rc1 (trunk) r62370.

(18) Conditional jump or move depends on uninitialised value(s)

==12324== Conditional jump or move depends on uninitialised value(s)
==12324==    at 0x45FF75: dc_add_history_dbl (in /home/sasha/zabbix-svn/trunk/sbin/zabbix_server)
==12324==    by 0x4604B7: DCmass_add_history (in /home/sasha/zabbix-svn/trunk/sbin/zabbix_server)
==12324==    by 0x461763: DCsync_history (in /home/sasha/zabbix-svn/trunk/sbin/zabbix_server)
==12324==    by 0x41C65F: dbsyncer_thread (in /home/sasha/zabbix-svn/trunk/sbin/zabbix_server)
==12324==    by 0x48DAF4: zbx_thread_start (in /home/sasha/zabbix-svn/trunk/sbin/zabbix_server)
==12324==    by 0x41B68A: MAIN_ZABBIX_ENTRY (in /home/sasha/zabbix-svn/trunk/sbin/zabbix_server)
==12324==    by 0x48C4C0: daemon_start (in /home/sasha/zabbix-svn/trunk/sbin/zabbix_server)
==12324==    by 0x41AF7F: main (in /home/sasha/zabbix-svn/trunk/sbin/zabbix_server)

==12323== Conditional jump or move depends on uninitialised value(s)
==12323==    at 0x46006B: dc_add_history_uint (in /home/sasha/zabbix-svn/trunk/sbin/zabbix_server)
==12323==    by 0x4604CE: DCmass_add_history (in /home/sasha/zabbix-svn/trunk/sbin/zabbix_server)
==12323==    by 0x461763: DCsync_history (in /home/sasha/zabbix-svn/trunk/sbin/zabbix_server)
==12323==    by 0x41C65F: dbsyncer_thread (in /home/sasha/zabbix-svn/trunk/sbin/zabbix_server)
==12323==    by 0x48DAF4: zbx_thread_start (in /home/sasha/zabbix-svn/trunk/sbin/zabbix_server)
==12323==    by 0x41B68A: MAIN_ZABBIX_ENTRY (in /home/sasha/zabbix-svn/trunk/sbin/zabbix_server)
==12323==    by 0x48C4C0: daemon_start (in /home/sasha/zabbix-svn/trunk/sbin/zabbix_server)
==12323==    by 0x41AF7F: main (in /home/sasha/zabbix-svn/trunk/sbin/zabbix_server)

glebs.ivanovskis RESOLVED in r62379.

sasha CLOSED

Subissue (18) is fixed in pre-3.2.0rc1 (trunk) r62386.

Please approve and resolve or reopen if any other documentation on sending history data is needed.

Open doc. subtasks (4) and (15) might be closed if text description meets provided code example, so these have to be reviewed by a dev.