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

Enable passing input file, output file, and mibs dir on command line … #1028

Merged
merged 7 commits into from
Nov 10, 2023

Conversation

dnck
Copy link
Contributor

@dnck dnck commented Oct 18, 2023

Description

@SuperQ @RichiH 👋

Hi there, I think this is a fairly trivial PR, but lmk what you think.

What's it do?

In order to facilitate the organization of snmp.ymls in different directories, I wanted a way to specify 1) the path to the
generator.yml, 2) the expected output snmp.yml, and 3) the directory of mibs.

For this reason, I added a few flags to the cli generator generate command group.

With the PR, you can do something like,

  ./generator generate \
    -g snmp-exporter/generator/myDevices/newManufacturer/generator.yml \
    -o snmp-exporter/generator/myDevices/newManufacturer/snmp.yml \ 
    -m snmp-exporter/generator/myDevices/shared-mibs \
    -m snmp-exporter/generator/myDevices/newManufacturer

which is helpful for me, and I hope for some others as well.

Copy link
Member

@SuperQ SuperQ left a comment

Choose a reason for hiding this comment

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

I'm generally in favor.

What do you think about implementing this with Strings() so that multiple files can be supported? Similar to what we now do in the exporter itself?

@dnck
Copy link
Contributor Author

dnck commented Oct 20, 2023

I'm generally in favor.

What do you think about implementing this with Strings() so that multiple files can be supported? Similar to what we now do in the exporter itself?

I think that makes sense for the --mibs-dir option, for sure. I can add that!

But, would it even make sense for the --generator-path and --output-path flags? Probably not. Unless I'm missing something?

@SuperQ
Copy link
Member

SuperQ commented Oct 20, 2023

For Generator path, yea, I think multi-file would be nice. Output, no, a single output file is fine.

C.netsnmp_set_mib_directory(C.CString(*userMibsDir))
} else {
// Help the user find their MIB directories.
err = level.Info(logger).Log("msg", "Loading MIBs", "from", C.GoString(C.netsnmp_get_mib_directory()))
Copy link
Member

Choose a reason for hiding this comment

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

We don't need to worry about err from the logger. The error as actually never tripped due to the way the library works. This is why we ignore this in the golanci-lint config in the project.

Also, do we need to duplicate the log lines? Shouldn't C.netsnmp_get_mib_directory() return the correct thing after C.netsnmp_set_mib_directory()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The error as actually never tripped due to the way the library works.

🤦 Thanks! I'll fix those places where I somewhat mindlessly checked for the err.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Shouldn't C.netsnmp_get_mib_directory() return the correct thing after C.netsnmp_set_mib_directory()?

Hmm, I'll check that. I would expect that it should.

@dnck
Copy link
Contributor Author

dnck commented Nov 6, 2023

@SuperQ - Thanks for your patience. I think this is now ready for another review.

@@ -384,7 +393,10 @@ func generateConfigModule(cfg *ModuleConfig, node *Node, nameToNode map[string]*
if prevType == subtype {
metric.Indexes = metric.Indexes[:len(metric.Indexes)-1]
} else {
level.Warn(logger).Log("msg", "Can't handle index type on node, missing preceding", "node", n.Label, "type", index.Type, "missing", subtype)
err := level.Warn(logger).Log("msg", "Can't handle index type on node, missing preceding", "node", n.Label, "type", index.Type, "missing", subtype)
Copy link
Member

Choose a reason for hiding this comment

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

Same with these, please remove all the extraneous err checks you've added.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I got them all now

Copy link
Member

@SuperQ SuperQ left a comment

Choose a reason for hiding this comment

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

Thanks!

@SuperQ SuperQ merged commit 6c92a3f into prometheus:main Nov 10, 2023
2 checks passed
SuperQ added a commit that referenced this pull request Nov 24, 2023
* [ENHANCEMENT] generator: Add support for subsequent address family #782
* [ENHANCEMENT] generator: Fix lookups to match OIDs closer to the index OID. #828
* [FEATURE] Add a scaling factor #1026
* [FEATURE] generator: Enable passing input file, output file, and mibs dir as flags #1028
* [FEATURE] Add an offset factor #1029
* [BUGFIX] Fix and optimize generator Docker image building #1045

snmp.yml changes:

* Override `bsnAPName` to DisplayString #660
* Import TP-Link EAP MIB  #833
* Updated Mikrotik neighbor indexes make them unique #986
* Update PowerNet MIB to v4.5.1 #1003
* Refactor HOST-RESOURCES-MIB #1027
* Update keepalived MIB files to latest version #1044

Signed-off-by: SuperQ <[email protected]>
@SuperQ SuperQ mentioned this pull request Nov 24, 2023
SuperQ added a commit that referenced this pull request Nov 26, 2023
* [ENHANCEMENT] generator: Add support for subsequent address family #782
* [ENHANCEMENT] generator: Fix lookups to match OIDs closer to the index OID. #828
* [FEATURE] Add a scaling factor #1026
* [FEATURE] generator: Enable passing input file, output file, and mibs dir as flags #1028
* [FEATURE] Add an offset factor #1029
* [BUGFIX] Fix and optimize generator Docker image building #1045

snmp.yml changes:

* Override `bsnAPName` to DisplayString #660
* Import TP-Link EAP MIB  #833
* Updated Mikrotik neighbor indexes make them unique #986
* Update PowerNet MIB to v4.5.1 #1003
* Refactor HOST-RESOURCES-MIB #1027
* Update keepalived MIB files to latest version #1044

Signed-off-by: SuperQ <[email protected]>
SuperQ added a commit that referenced this pull request Dec 6, 2023
* [ENHANCEMENT] generator: Add support for subsequent address family #782
* [ENHANCEMENT] generator: Fix lookups to match OIDs closer to the index OID. #828
* [FEATURE] Add a scaling factor #1026
* [FEATURE] generator: Enable passing input file, output file, and mibs dir as flags #1028
* [FEATURE] Add an offset factor #1029
* [BUGFIX] Fix and optimize generator Docker image building #1045

snmp.yml changes:

* Override `bsnAPName` to DisplayString #660
* Import TP-Link EAP MIB  #833
* Updated Mikrotik neighbor indexes make them unique #986
* Update PowerNet MIB to v4.5.1 #1003
* Refactor HOST-RESOURCES-MIB #1027
* Update keepalived MIB files to latest version #1044

Signed-off-by: SuperQ <[email protected]>
SuperQ added a commit that referenced this pull request Dec 10, 2023
* [ENHANCEMENT] generator: Add support for subsequent address family #782
* [ENHANCEMENT] generator: Fix lookups to match OIDs closer to the index OID. #828
* [FEATURE] Add a scaling factor #1026
* [FEATURE] generator: Enable passing input file, output file, and mibs dir as flags #1028
* [FEATURE] Add an offset factor #1029
* [BUGFIX] Fix and optimize generator Docker image building #1045

snmp.yml changes:

* Override `bsnAPName` to DisplayString #660
* Import TP-Link EAP MIB  #833
* Updated Mikrotik neighbor indexes make them unique #986
* Update PowerNet MIB to v4.5.1 #1003
* Refactor HOST-RESOURCES-MIB #1027
* Update keepalived MIB files to latest version #1044

Signed-off-by: SuperQ <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants