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/sqlserver] Metrics emitted with incorrect resource attributes #35036

Closed
crobert-1 opened this issue Sep 5, 2024 · 2 comments · Fixed by #35038
Closed

[receiver/sqlserver] Metrics emitted with incorrect resource attributes #35036

crobert-1 opened this issue Sep 5, 2024 · 2 comments · Fixed by #35038
Assignees
Labels
bug Something isn't working needs triage New item requiring triage receiver/sqlserver

Comments

@crobert-1
Copy link
Member

Component(s)

receiver/sqlserver

Describe the issue you're reporting

The bug is here:

Essentially what's happening is the receiver sets the resource attributes based on the first row, and all following rows are given the same resource attributes. However, this is a bug as the database_name, a resource attribute, changes for different rows. The result is that all of the metrics are simply considered to be a part of the first database name, rather than their correct value.

There could be two main ways to fix this:

  1. Emit the given resource on each row. This will be the least impactful option, but results in multiple resource metrics having the same resource attributes.
  2. Create an in-memory data structure to represent the resource attributes, and deduplicate when going row-by-row. The logic would be considerably more complicated, but it would result in a single resource metric for a set of resource attributes.

My general preference would be to go with option number 1 as it's simpler and less prone to errors, but I'm not sure if this breaks the required format of emitted metrics.

Note: This is a result of #30297, and only relevant for direct connection configurations.

@crobert-1 crobert-1 added the needs triage New item requiring triage label Sep 5, 2024
Copy link
Contributor

github-actions bot commented Sep 5, 2024

Pinging code owners:

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

@crobert-1
Copy link
Member Author

I've posted #35038 implementing my proposed approach option 1. View the updated testdata files to see the resulting resources. I'd appreciate input on whether this is the right or wrong approach to solve this bug.

@mx-psi mx-psi closed this as completed in e9b835f Sep 6, 2024
f7o pushed a commit to f7o/opentelemetry-collector-contrib that referenced this issue Sep 12, 2024
…pen-telemetry#35038)

**Description:** <Describe what has changed.>
<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue.
Ex. Adding a feature - Explain what this achieves.-->
As explained in the bug description, the receiver was incorrectly
setting the database name resource attribute based on the first row
returned from the query. The returned rows may have different database
names, which means metrics were being labeled as being from the wrong
database.

**Link to tracking Issue:** <Issue number if applicable>
Fixes
open-telemetry#35036

**Testing:** <Describe what testing was performed and which tests were
added.>
Updated tests for new expected resource metrics. View changed
`testdata/` files to see changed output.
djaglowski pushed a commit that referenced this issue Sep 30, 2024
… metrics (#35040)

**Description:** <Describe what has changed.>
<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue.
Ex. Adding a feature - Explain what this achieves.-->
The computer name resource attribute is relevant for all metrics coming
from the SQL server receiver when directly connected to the DB. This
change records the resource attribute with all remaining metrics that it
wasn't recorded with yet.

**Link to tracking Issue:** <Issue number if applicable>
This was discovered at the same time as
#35036,
but I split out the changes to hopefully make them easier to review.

**Testing:** <Describe what testing was performed and which tests were
added.>
Tests have been updated and are passing.
jriguera pushed a commit to springernature/opentelemetry-collector-contrib that referenced this issue Oct 4, 2024
…pen-telemetry#35038)

**Description:** <Describe what has changed.>
<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue.
Ex. Adding a feature - Explain what this achieves.-->
As explained in the bug description, the receiver was incorrectly
setting the database name resource attribute based on the first row
returned from the query. The returned rows may have different database
names, which means metrics were being labeled as being from the wrong
database.

**Link to tracking Issue:** <Issue number if applicable>
Fixes
open-telemetry#35036

**Testing:** <Describe what testing was performed and which tests were
added.>
Updated tests for new expected resource metrics. View changed
`testdata/` files to see changed output.
jriguera pushed a commit to springernature/opentelemetry-collector-contrib that referenced this issue Oct 4, 2024
… metrics (open-telemetry#35040)

**Description:** <Describe what has changed.>
<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue.
Ex. Adding a feature - Explain what this achieves.-->
The computer name resource attribute is relevant for all metrics coming
from the SQL server receiver when directly connected to the DB. This
change records the resource attribute with all remaining metrics that it
wasn't recorded with yet.

**Link to tracking Issue:** <Issue number if applicable>
This was discovered at the same time as
open-telemetry#35036,
but I split out the changes to hopefully make them easier to review.

**Testing:** <Describe what testing was performed and which tests were
added.>
Tests have been updated and are passing.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working needs triage New item requiring triage receiver/sqlserver
Projects
None yet
1 participant