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/snmp] Add column and scalar OID metrics to resources that have scalar OID attributes #25174

Conversation

kuiperda
Copy link
Contributor

@kuiperda kuiperda commented Aug 11, 2023

Description:

  • Added a new scalar_oid field to ResourceAttribtueConfigs to allow Scalar OIDs to be used as resource attributes (on Column OID metrics)
  • Added support for (Scalar OID only) resource attributes on Scalar OID metrics as well.

Link to tracking Issue:
#23373

Testing:

  • Valid and invalid configs using a scalar_oid resource attribute on both scalar and column oid metrics
  • Adding indexed metrics to resources with scalar_oid resource attributes
  • Adding scalar metrics to resources with scalar_oid resource attributes

Documentation:

  • Description of new scalar_oid field in ResourceAttributeConfig and comments on new functions
  • Description of new ResourceAttributes field on Scalar OID metrics

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Aug 11, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

receiver/snmpreceiver/config.go Outdated Show resolved Hide resolved
receiver/snmpreceiver/config.go Outdated Show resolved Hide resolved
receiver/snmpreceiver/config.go Outdated Show resolved Hide resolved
receiver/snmpreceiver/config.go Outdated Show resolved Hide resolved
receiver/snmpreceiver/config.go Show resolved Hide resolved
receiver/snmpreceiver/config.go Outdated Show resolved Hide resolved
receiver/snmpreceiver/config.go Outdated Show resolved Hide resolved
receiver/snmpreceiver/config.go Outdated Show resolved Hide resolved
receiver/snmpreceiver/README.md Show resolved Hide resolved
receiver/snmpreceiver/README.md Outdated Show resolved Hide resolved
receiver/snmpreceiver/config.go Outdated Show resolved Hide resolved
receiver/snmpreceiver/config.go Show resolved Hide resolved
receiver/snmpreceiver/config.go Outdated Show resolved Hide resolved
receiver/snmpreceiver/config.go Outdated Show resolved Hide resolved
@kuiperda kuiperda force-pushed the feat/snmpreceiver-resource-attribute-updates branch from 802cd8e to b177fa9 Compare August 21, 2023 20:03
@kuiperda kuiperda marked this pull request as ready for review August 21, 2023 20:07
@kuiperda kuiperda requested a review from a team August 21, 2023 20:07
receiver/snmpreceiver/config.go Outdated Show resolved Hide resolved
receiver/snmpreceiver/config_helper.go Outdated Show resolved Hide resolved
receiver/snmpreceiver/config_helper.go Outdated Show resolved Hide resolved
receiver/snmpreceiver/scraper.go Show resolved Hide resolved
receiver/snmpreceiver/scraper.go Outdated Show resolved Hide resolved
receiver/snmpreceiver/scraper.go Outdated Show resolved Hide resolved
receiver/snmpreceiver/scraper.go Outdated Show resolved Hide resolved
@djaglowski
Copy link
Member

Please address CI failures. Feel free to ping me to run actions after you push.

@djaglowski djaglowski changed the title Feat/snmpreceiver resource attribute updates [receiver/snmp] Add column and scalar OID metrics to resources that have scalar OID attributes Aug 29, 2023
Copy link
Member

@djaglowski djaglowski left a comment

Choose a reason for hiding this comment

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

LGTM. Just a couple very minor questions about error propagation.

receiver/snmpreceiver/config.go Show resolved Hide resolved
receiver/snmpreceiver/config.go Show resolved Hide resolved
@djaglowski djaglowski merged commit ba8cfce into open-telemetry:main Aug 29, 2023
@github-actions github-actions bot added this to the next release milestone Aug 29, 2023
@kuiperda kuiperda deleted the feat/snmpreceiver-resource-attribute-updates branch August 29, 2023 18:02
djaglowski pushed a commit that referenced this pull request Sep 5, 2023
**Description:** 
Fixed a bug introduced by
[#25174](#25174)

There was check that was supposed to ensure that all resource attributes
added to a metric were scalar, but it was really checking if the number
of resource attributes on a metric was the same as the total number of
possible scalar resource attributes defined in the config. This check
has been updated to explicitly verify that all of the resource
attributes on the metric are scalar.

Bug examples:
- A column metric indexed by a column attribute has two scalar resource
attributes with different names but the same oid value. They are the
only two scalar RAs defined in the config. Since the oids are added to a
map whose length is checked, they are seen as one value and the
comparison fails so many resources are created instead of one.
- A column metric indexed by a column attribute has only one scalar
resource attribute when two possible attributes are defined in the
config. The check fails and many resources are created instead of one.

**Testing:** 
- Test that a metric with one scalar resource attribute when two are
available still creates only one resource
- First bug does not feel necessary to unit test, it has been tested
locally and is fixed.

**Documentation:**
- N/A
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