Centos 6.5, Oracle Instant Client, unixODBC

Sorry, field names in JSON must be:

Oleksiy, will this feature implemented in 2.2.3(4)?

Feature implemented at svn://svn.zabbix.com/branches/dev/ZBXNEXT-2321

Juris, in which version this feature will be realesed? Any docs?

Vadim, it is currently in the testing phase and will be merged into trunk as soon as it's gone through testing and reviews. It will be merged in trunk, and stabily it will become available in 3.0.

(1) Please review stylistic and warning fixes in r50528. Warnings complained about unused variables, other changes try to make new code more consistent with the rest.

jurism Looks fine. RESOLVED.

asaveljevs CLOSED

(2) There should be an error message in case column name has a character that is unsuitable for a macro name. Currently, there is no message.

jurism Error message added. RESOLVED.

asaveljevs CLOSED

(3) We currently have the following code:

while (NULL != (row = odbc_DBfetch(&dbh)))
{
	zbx_json_addobject(&json, NULL);

	for (i = 0; i < dbh.col_num; i++)
	{
		zbx_snprintf(colname, MAX_STRING_LEN, "{#%s}", column_names[i]);
		zbx_json_addstring(&json, colname, row[i], ZBX_JSON_TYPE_STRING);
		zbx_free(column_names[i]);
	}

	zbx_json_close(&json);
}

Something tells me "column_names[i]" should not be freed.

jurism Column names are now freed when they're not necessary anymore, as opposed to after the first usage. RESOLVED.

asaveljevs CLOSED

(4) It would be nice to have a specification for this development. For instance, it should be specified how column names from the following queries are converted to macro names (if at all):

select field as f ...
select count(*) ...
select t1.field, t2.field ...
select field, NULL ...

asaveljevs Specification is in progress. What follows is a table that describes how columns in these queries are handled by SQLColAttribute() with SQL_DESC_LABEL:

asaveljevs Specification ready for review at https://www.zabbix.org/wiki/Docs/specs/ZBXNEXT-2321 . RESOLVED.

asaveljevs Question regarding JSON null in filters moved to ZBX-9146. CLOSED.

(5) ZBXNEXT-2203 increased error message length for items from 128 to 2048 characters. However, the following pages still refer to 128-character limit, which should be fixed:

• https://www.zabbix.com/documentation/2.4/manual/config/items/itemtypes/odbc_checks
• https://www.zabbix.com/documentation/3.0/manual/config/items/itemtypes/odbc_checks

jurism
Documentation udpated. RESOLVED.

wiper CLOSED

This comment describes the way column name is obtained from the SQL query in get_result_table_column_names() function.

Internally, it uses SQLColAttribute() function. According to http://msdn.microsoft.com/en-us/library/ms713558(v=vs.85).aspx ODBC documentation, there are three relevant parameters that can be passed to obtain column name: SQL_DESC_BASE_COLUMN_NAME, SQL_DESC_LABEL, SQL_DESC_NAME.

The first option, SQL_DESC_BASE_COLUMN_NAME, disregards aliases, so "select field as f ..." gives "field" and "select count(*) as c ..." gives an empty string. This option is thus unusable for our purposes.

The second two options, SQL_DESC_LABEL and SQL_DESC_NAME, have been tested on MySQL, PostgreSQL, and Oracle, and the difference between the two has not been observed. Among these, SQL_DESC_LABEL seems better, because it comes from ODBC 2.0, while SQL_DESC_NAME comes from ODBC 3.0.

(6) Added new translation strings:

• Return first column of the first row of the SQL query result.
• Transform SQL query result into a JSON object for low-level discovery.

wiper CLOSED

(7) To avoid any possible issues with utf-8 names we should cast characters to unsigned char when passing them to isalpha/toupper functions.

asaveljevs RESOLVED in r51441.

wiper CLOSED

Server side successfully tested

(8) [F]

• CItemHelper.php:
  • The helper displays <description> instead of <unique short description> and selecting it also changes from <unique short description> to <description>. It's better to use one text like <unique short description> since has been already used before for other item types.
• configuration.item.edit.js.php: 67
  • please, use constants instead of hardcoded numbers
• popup.php: 203
  • line exceeds maximum length.
  • Would be nice to change also $allowed_item_types to camel case $allowedItemTypes. And by doing that, don't forget to change $_REQUEST to getRequest() in following lines.
• common.item.edit.js.php: 111
  • avoid using empty()
  • redundant parenthesis (empty....DISCOVERY);
  • line exceeds maximum length
• Selenium tests testFormItem.php and testFormItemPrototype.php probably need to be updated, since 'Database monitor' no longer has only one key.
  I assume this piece

  if ($type == 'Database monitor' && !isset($itemid)) {
  	$keyValue = $this->getValue('key');
  	$this->assertEquals($keyValue, "db.odbc.select[<unique short description>,<dsn>]");
  }
  

  is now obsolete. Please, check.

asaveljevs Regarding tests, item key for regular database items and for item prototypes is still prefilled with "db.odbc.select[]". It is only prefilled with "db.odbc.discovery[]" for discovery rules. Therefore, the test still seems to be valid.

asaveljevs Other suggestions RESOLVED in r51537.

iivs Thanks! I refactored the code a bit. Please see r51541

asaveljevs Looks good. CLOSED.

(9) [F] LLD rule form choosing "Database monitor" it selects default example "db.odbc.discovery[<unique short description>,<dsn>]", this also should be checked in CItemGeneral.php: 283
and display error "Check the key, please. Default example was passed." in case default example is passed.

asaveljevs RESOLVED in r51548.

iivs

Undefined index: flags [items.php:505 → CFrontendApiWrapper->create() → CApiWrapper->__call() → CFrontendApiWrapper->callMethod() → CApiWrapper->callMethod() → CFrontendApiWrapper->callClientMethod() → CLocalApiClient->callMethod() → call_user_func_array() → CItem->create() → CItemGeneral->checkInput() in C:\Development\ZBXNEXT-2321-test\frontends\php\include\classes\api\services\CItemGeneral.php:288]

• "elseif" must be written as one word;
• there are two "if" on line 283 and 293, both will execute, but there is no reason, we could use "elseif" there.

REOPENED.

asaveljevs RESOLVED in r51551.

iivs CLOSED.

Frontend TESTED.

Available in pre-2.5.0 (trunk) r51577.

(10) Documented since which versions "system.cpu.discovery" and SNMP discovery are supported:

• https://www.zabbix.com/documentation/2.0/manual/discovery/low_level_discovery
• https://www.zabbix.com/documentation/2.2/manual/discovery/low_level_discovery
• https://www.zabbix.com/documentation/2.4/manual/discovery/low_level_discovery
• https://www.zabbix.com/documentation/3.0/manual/discovery/low_level_discovery

wiper CLOSED

(11) Documented ODBC LLD functionality at the following locations:

• https://www.zabbix.com/documentation/3.0/manual/introduction/whatsnew300#item_changesimprovements
• https://www.zabbix.com/documentation/3.0/manual/discovery/low_level_discovery#discovery_using_odbc_sql_queries

For documentation purposes, a practical example from ZBXNEXT-2548 was chosen.

wiper I'd suggest creating separate section for ODBC discovery in what's new, but otherwise looks great.
CLOSED