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

Snmpreceiver fix counter64 #26368

Merged

Conversation

technimad-splunk
Copy link
Contributor

Description:
SNMP data with type Counter64 can be sent as a uint64. This type wasn't handled.
Internally we convert all numerical SNMP values to an int64.
The conversion for a uint64, sent by the newly supported Conter64 SNMP data type, to int64
was lacking and is introduced with this PR.

Link to tracking Issue:
#23897, PR #26119

Testing:
Ran the collector with a config to collect oid 1.3.6.1.2.1.31.1.1.1.6, which is a Counter64.
This didn't work, showed value type as unsupported.
After the change data from this oid comes in as expected.

Documentation: n/a

…'t handeled.

Internally we convert all numerical SNMP values to an int64.
The conversion for a uint64, sent by the newly supported Conter64 SNMP data type, to int64
was lacking and is intruduced with this commit.
@technimad-splunk technimad-splunk requested a review from a team September 1, 2023 08:21
@technimad-splunk technimad-splunk changed the title Snmpreceiver add counter64 Snmpreceiver fix counter64 Sep 1, 2023
@fatsheep9146
Copy link
Contributor

failed unit test should be fixed @technimad-splunk

@technimad-splunk
Copy link
Contributor Author

I'm still working on the tests.

@technimad-splunk
Copy link
Contributor Author

failed unit test should be fixed @technimad-splunk

Test are green now. Fixed by ed97de1

@@ -513,7 +513,7 @@ func TestGetScalarData(t *testing.T) {
expectedSNMPData := []SNMPData{}
mockGoSNMP := new(mocks.MockGoSNMPWrapper)
pdu1 := gosnmp.SnmpPDU{
Value: uint64(math.MaxUint64),
Value: float64(math.MaxFloat64),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why change this too?

Copy link
Contributor Author

@technimad-splunk technimad-splunk Sep 5, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a test for an invalid data type.
If we receive an invalid or unsopported data type an error should be raised.

With this PR, uint64 becomes supported, and no error is raised anymore, causing the test to fail.
This change tests for an other, yet unsupported, data type (float64) which should generate an error.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, missed the context. Thanks for explaining.

@djaglowski djaglowski merged commit e6e5f54 into open-telemetry:main Sep 5, 2023
90 checks passed
@github-actions github-actions bot added this to the next release milestone Sep 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants