[receiver/sqlserver] Properly parse scientific notation numbers to ints #39905
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-actions
bot
added
receiver/sqlserver
Run Windows
crobert-1
commented
May 6, 2025
| } | ||
| } | ||
| } else { | ||
| err = fmt.Errorf("no value found for column %s", columnName) |
There was a problem hiding this comment.
Technically in both retrieveInt and retrieveFloat I've added new potential errors scenarios, but both of these methods are only called currently from a select when a column name is seen in a row. I don't believe either of the new error cases are hit today, they're an attempt at future-proofing these methods.
Also, even if they're introducing new errors, the receiver is already failing to scrape the given metric or attribute, but it's just being hidden here and metrics are recorded as zero values. The proper behavior should be to notify users of errors.
atoulme
approved these changes
May 6, 2025
sincejune
approved these changes
May 7, 2025
crobert-1
added
ready to merge
crobert-1
force-pushed
the
sqlserver_scientific_notation
branch
from
3576b7e to
4cf22c1
Compare
crobert-1
changed the title
dragonlord93
pushed a commit
to dragonlord93/opentelemetry-collector-contrib
that referenced
this pull request
May 23, 2025
…ts (open-telemetry#39905) <!--Ex. Fixing a bug - Describe the bug and how this fixes the issue. Ex. Adding a feature - Explain what this achieves.--> #### Description SQL Server stores large integers in scientific e notation. `strconv.ParseInt` fails to parse this and raises an error, as seen in the original filed issue. For our use case, we still want these to be considered valid integers, so before failing entirely, attempt to parse to float and convert to integer. This change also uses already existing methods, `retrieveInt` and `retrieveFloat` to be able to re-use the parsing functionality. The original issue was only for `sqlserver.database.tempdb.space`, but I believe this issue is relevant for all metrics of type `int`. <!-- Issue number (e.g. open-telemetry#1234) or full URL to issue, if applicable. --> #### Link to tracking issue Fixes open-telemetry#39124 <!--Describe what testing was performed and which tests were added.--> #### Testing Changed test values of a couple integers to ensure new functionality works. The changed values broke tests on `main` in the same way the filed bug shows, but tests are passing with this change.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
SQL Server stores large integers in scientific e notation.
strconv.ParseIntfails to parse this and raises an error, as seen in the original filed issue. For our use case, we still want these to be considered valid integers, so before failing entirely, attempt to parse to float and convert to integer.This change also uses already existing methods,
retrieveIntandretrieveFloatto be able to re-use the parsing functionality. The original issue was only forsqlserver.database.tempdb.space, but I believe this issue is relevant for all metrics of typeint.Link to tracking issue
Fixes #39124
Testing
Changed test values of a couple integers to ensure new functionality works. The changed values broke tests on
mainin the same way the filed bug shows, but tests are passing with this change.