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.

Sign up for GitHub

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

Jump to bottom

Add support for RFC 2579 DateAndTime #322

Merged
merged 4 commits into from
Aug 15, 2018
Merged

Add support for RFC 2579 DateAndTime #322

merged 4 commits into from
Aug 15, 2018

Conversation

SuperQ
Copy link
Member

@SuperQ SuperQ commented Aug 14, 2018 •
edited
Loading

Extract a UNIX timestamp from RFC 2579 DateAndTime textual conversions.

  • Add support to the generator.
  • Add support to the collector.
  • Update snmp.yml with new DateAndTime metrics.

Fixes: #321

Signed-off-by: Ben Kochie [email protected]

@SuperQ SuperQ requested a review from brian-brazil August 14, 2018 13:57
RichiH
RichiH reviewed Aug 14, 2018
View reviewed changes
generator/tree.go
@@ -130,6 +137,8 @@ func metricType(t string) (string, bool) {
return "IpAddr", true
case "PhysAddress48", "DisplayString", "Float", "Double":
return t, true
case "DateAndTime":
Copy link
Member

Choose a reason for hiding this comment

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

Are you certain that's the only way to do datetime? Maybe call it RFC2579DateAndTime or something?

Copy link
Member Author

Choose a reason for hiding this comment

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

This comes from the definition in the MIB/RFC.

DateAndTime ::= TEXTUAL-CONVENTION

Copy link
Member

Choose a reason for hiding this comment

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

Ah, good.

Copy link
Contributor

Choose a reason for hiding this comment

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

The textual convention is what we use for these, so this is fine.

brian-brazil
brian-brazil reviewed Aug 14, 2018
View reviewed changes
Copy link
Contributor

@brian-brazil brian-brazil left a comment

Choose a reason for hiding this comment

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

The readme needs an update for the new type

collector.go Outdated
// No time zone included, assume UTC.
tz = time.UTC
case 11:
// Extract the timezone from hte last 3 bytes.
Copy link
Contributor

Choose a reason for hiding this comment

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

the

collector.go Outdated
return 0
}
if err != nil {
log.Debugf("unable to parse DateAndTime %q", v)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you log the error?

collector.go Outdated
v = pdu.Value.([]byte)
default:
log.Debugf("invalid DateAndTime type %v", pduType)
return 0
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we really be returning 0 here?

Copy link
Member Author

Choose a reason for hiding this comment

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

I was debating how to do error handling here. The function that uses this, pduToSamples, doesn't really have any error handling.

Copy link
Contributor

Choose a reason for hiding this comment

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

We can either fail the scrape, or return nothing for this metric and log. Returning bad data is not a good option imho.

generator/tree.go Outdated
@@ -115,6 +115,13 @@ func prepareTree(nodes *Node) map[string]*Node {
}
})

// Convert RFC 2579 DateAndTime textual conversion to type.
walkNode(nodes, func(n *Node) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This and the previous two walks should really be combined

Copy link
Member Author

Choose a reason for hiding this comment

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

I saw this, and was wondering why we did walkNode over more than once. I can refactor.

@SuperQ SuperQ force-pushed the superq/DateAndTime branch 2 times, most recently from 3459c18 to e041e60 Compare August 14, 2018 15:34
brian-brazil
brian-brazil reviewed Aug 14, 2018
View reviewed changes
generator/README.md Outdated
@@ -105,6 +105,7 @@ modules:
# gauge: An integer with type gauge.
# counter: An integer with type counter.
# OctetString: A bit string, rendered as 0xff34.
# DateAndTime: An RFC 2579 DateAndTime byte sequence.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd mention that UTC is presumed if the tz is missing.

@SuperQ SuperQ force-pushed the superq/DateAndTime branch from e041e60 to bd94ce9 Compare August 14, 2018 15:53
@brian-brazil
Copy link
Contributor

You need unittests for both the collector and generator changes.

@SuperQ SuperQ force-pushed the superq/DateAndTime branch from d583f85 to 8a031d0 Compare August 15, 2018 08:53
@SuperQ
Copy link
Member Author

SuperQ commented Aug 15, 2018

@brian-brazil Yup, tests were why I left it WIP.

@SuperQ SuperQ changed the title WIP: Add support for RFC 2579 DateAndTime Add support for RFC 2579 DateAndTime Aug 15, 2018
brian-brazil
brian-brazil reviewed Aug 15, 2018
View reviewed changes
generator/FORMAT.md Outdated
@@ -25,6 +26,10 @@ module_name:
# as a label value on that gauge.

# A metric that's part of a table, and thus has labels.
Copy link
Contributor

Choose a reason for hiding this comment

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

This isn't part of a table. You don't need to change this file.

Copy link
Member Author

Choose a reason for hiding this comment

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

I was thinking it would be worth having an example in the FORMAT file. Don't care either way.

Copy link
Contributor

Choose a reason for hiding this comment

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

The format file is more the general structure, the various types are over in the readme.

Copy link
Member Author

Choose a reason for hiding this comment

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

Got it, removed.

Copy link
Contributor

Choose a reason for hiding this comment

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

The types used to be in here, but they got moved when the type override was added. I'd prefer not to having to maintain two lists of types, as it's more likely they'll get out of sync.

@SuperQ SuperQ force-pushed the superq/DateAndTime branch from 8a031d0 to 01e327b Compare August 15, 2018 09:15
SuperQ added 4 commits August 15, 2018 11:16
@SuperQ
Add support for RFC 2579 DateAndTime
7ef3b9c 
Extract a UNIX timestamp from RFC 2579 DateAndTime textual conversions.
* Add support to the generator.
* Add support to the collector.
* Update snmp.yml with new DateAndTime metrics.

Signed-off-by: Ben Kochie <[email protected]>
@SuperQ
Refactor type walkNode in generator/tree.go.
4cbe59f 
Signed-off-by: Ben Kochie <[email protected]>
@SuperQ
Add documentation.
da9f183 
Signed-off-by: Ben Kochie <[email protected]>
@SuperQ
Add tests
64ccfdc 
Signed-off-by: Ben Kochie <[email protected]>
@SuperQ SuperQ force-pushed the superq/DateAndTime branch from 01e327b to 64ccfdc Compare August 15, 2018 09:16
@brian-brazil brian-brazil merged commit 8c6195f into master Aug 15, 2018
@brian-brazil brian-brazil deleted the superq/DateAndTime branch August 15, 2018 09:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@brian-brazil brian-brazil brian-brazil left review comments

@RichiH RichiH RichiH left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@SuperQ @brian-brazil @RichiH