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

[receiver/sqlquery] Improve error handling for missing columns in result set #35068

Closed
Grandys opened this issue Sep 8, 2024 · 3 comments · Fixed by #35189
Closed

[receiver/sqlquery] Improve error handling for missing columns in result set #35068

Grandys opened this issue Sep 8, 2024 · 3 comments · Fixed by #35189

Comments

@Grandys
Copy link
Contributor

Grandys commented Sep 8, 2024

Component(s)

internal/sqlquery, receiver/sqlquery

Describe the issue you're reporting

I have two suggestions how to improve error handling:

  1. The value and body columns do not match the column names in the result set, handling is different for logs and metrics. Here is the problematic configuration:
  sqlquery:
...
      - sql: "select max(datapoint) as datapoint from logs_data group by log_type"
        metrics:
          - metric_name: logs.datapoint
            value_column: "datapoint_ne"
      - sql: "select 'Max datapoint value for ' || log_type || ' is ' || max(datapoint) as body from logs_data group by log_type"
        logs:
          - body_column: body_ne

When I run the Otel collector with this configuration, logs are generated (with an empty body), but metrics are not. Below is the collector's output:

{
  "kind": "receiver",
  "name": "sqlquery",
  "data_type": "metrics",
  "error": "row 0: rowToMetric: value_column 'datapoint_ne' not found in result set\nrow 1: rowToMetric: value_column 'datapoint_ne' not found in result set\nrow 2: rowToMetric: value_column 'datapoint_ne' not found in result set",
  "scraper": "sqlqueryreceiver/query-0: select max(datapoint) as datapoint from logs_data group by log_type"
}

Notably, there is no error entry for "data_type": "logs". Ideally, the handling of missing body columns for logs should be consistent with the handling of missing value columns for metrics. If we choose to fail on a missing body column, it would be a breaking change.

  1. The attribute_columns do not match the column names in the result set, error is logged only for the first miss. Here is the faulty configuration::
  sqlquery:
...
    queries:
      - sql: "select 'Max datapoint value for ' || log_type || ' is ' || max(datapoint) as datapoint, log_type from logs_data group by log_type"
        logs:
          - body_column: datapoint
            attribute_columns: [ "log_type_ne_1", "log_type_ne_2" ]

When the collector starts, the following error is logged:

{
  "kind": "receiver",
  "name": "sqlquery",
  "data_type": "logs",
  "error": "rowToLog: attribute_column not found: 'log_type_ne_1'\nrowToLog: attribute_column not found: 'log_type_ne_1'\nrowToLog: attribute_column not found: 'log_type_ne_1'",
  "query": "query-0: select 'Max datapoint value for ' || log_type || ' is ' || max(datapoint) as datapoint, log_type from logs_data group by log_type"
}

Although both attribute columns are missing from the result set, the error is logged only for the first missing attribute column. Providing more detailed error messages would enhance the debugging experience.

@Grandys Grandys added the needs triage New item requiring triage label Sep 8, 2024
Copy link
Contributor

github-actions bot commented Sep 8, 2024

Pinging code owners:

See Adding Labels via Comments if you do not have permissions to add labels yourself.

@Grandys Grandys changed the title Improve error handling for missing columns in result set [receiver/sqlquery] Improve error handling for missing columns in result set Sep 8, 2024
@Grandys
Copy link
Contributor Author

Grandys commented Sep 14, 2024

@crobert-1 @dmitryax
If you agree with my points, here is the PR: #35189

@crobert-1
Copy link
Member

This seems like a good idea to me, removing needs triage.

@crobert-1 crobert-1 removed the needs triage New item requiring triage label Sep 16, 2024
andrzej-stencel pushed a commit that referenced this issue Sep 27, 2024
…35189)

**Description:** 

* [breaking] Fail if for log column not found in result set (for
consistency with metrics behaviour)
* Instead of fail-fast, collect all errors that occurred when
transforming row to metric or log

**Link to tracking Issue:** #35068

**Testing:** Added/updated unit tests

**Documentation:** n/a

Closes #35068
jriguera pushed a commit to springernature/opentelemetry-collector-contrib that referenced this issue Oct 4, 2024
…pen-telemetry#35189)

**Description:** 

* [breaking] Fail if for log column not found in result set (for
consistency with metrics behaviour)
* Instead of fail-fast, collect all errors that occurred when
transforming row to metric or log

**Link to tracking Issue:** open-telemetry#35068

**Testing:** Added/updated unit tests

**Documentation:** n/a

Closes open-telemetry#35068
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants