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

Wrong behavior of outputs.sql.convert unsigned setting #10671

Closed
crabvk opened this issue Feb 17, 2022 · 2 comments · Fixed by #10673
Closed

Wrong behavior of outputs.sql.convert unsigned setting #10671

crabvk opened this issue Feb 17, 2022 · 2 comments · Fixed by #10673
Assignees
Labels
bug unexpected problem or unintended behavior

Comments

@crabvk
Copy link

crabvk commented Feb 17, 2022

Relevant telegraf.conf

[[outputs.sql]]
  driver = "clickhouse"
  data_source_name = "tcp://example.com:9000?username=user&password=pass&database=db"
  table_template = "CREATE TABLE {TABLE}({COLUMNS}) ENGINE = MergeTree() ORDER by timestamp"
  [outputs.sql.convert]
    integer = "Int64"
    real = "Float64"
    text = "String"
    timestamp = "DateTime"
    defaultvalue = "String"
    unsigned = "UInt64"
    bool = "Uint8"

Logs from Telegraf

2022-02-14T06:01:11Z E! [agent] Error writing to outputs.sql: code: 62, message: Syntax error: failed at position 72 ('UInt64'): UInt64,"inactive" Int64 UInt64,"vmalloc_chunk" Int64 UInt64,"sreclaimable" Int64 UInt64,"swap_cached" Int64 UInt64,"used" Int64 UInt64,"huge_pages_free" Int64 U. Expected one of: DEFAULT, MATERIALIZED, ALIAS, NOT, NULL, COMMENT, CODEC, TTL, token, Comma, ClosingRoundBracket

System info

Telegraf built from master branch (with #9671 PR merged), ArchLinux 5.16.8

Steps to reproduce

  1. Use telegraf config above
  2. Run telegraf to collect metrics to ClickHouse
  3. Got the error

Expected behavior

No errors

Actual behavior

This line causes the error, because when value type is uint64 the datatype becomes Int64 UInt64.

Additional info

Setting unsigned is exceptional, in fact it behaves more like unsigned_suffix now. To fix the issue the code should be

datatype = p.Convert.Unsigned

as with other data type settings, and default config for setting unsigned should be changed to

[outputs.sql.convert]
    unsigned = "INT UNSIGNED"

instead of

[outputs.sql.convert]
    unsigned = "UNSIGNED"

UNSIGNED is not a type on its own, it is a suffix.

@crabvk crabvk added the bug unexpected problem or unintended behavior label Feb 17, 2022
powersj added a commit to powersj/telegraf that referenced this issue Feb 17, 2022
@powersj
Copy link
Contributor

powersj commented Feb 17, 2022

Hi @crabvk,

Would you take a look at this PR and try one of the artifacts that are attached (once tests are built).

Thanks!

@crabvk crabvk closed this as completed Feb 17, 2022
@powersj
Copy link
Contributor

powersj commented Feb 17, 2022

Thank you for confirming in the PR that this fixes things. I am going to re-open this since my PR isn't merge quite yet :)

@powersj powersj reopened this Feb 17, 2022
@powersj powersj self-assigned this Feb 17, 2022
powersj added a commit to powersj/telegraf that referenced this issue Feb 17, 2022
This more accurately refelcts the correct naming for unsigned datatypes.

Additionally, this actually adds tests for the uint64 and float64
datatypes across clickhouse, mariadb, and postgres. Because postgres
does not have support for unsigned values, the number is treated as a
bigint.

Fixes: influxdata#10671
powersj added a commit to powersj/telegraf that referenced this issue Feb 24, 2022
Allow the user to specify a specific unsigned value to use for
converstion rather than assume the integer value + unsigned. This is
done via a new option.

Additionally, this actually adds tests for the uint64 and float64
datatypes across clickhouse, mariadb, and postgres. Because postgres
does not have support for unsigned values, the number is treated as a
bigint.

Fixes: influxdata#10671
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug unexpected problem or unintended behavior
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants