feat(snmp-telemetry): add the backend to the monorepo with CI and embedded profiles - #564
feat(snmp-telemetry): add the backend to the monorepo with CI and embedded profiles#564leoparente wants to merge 127 commits into
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
Go test coverage
Total coverage: 89.5% |
Vulnerability Scan: Failed — blocking vulnerabilities detectedImage:
Commit: b3c6073 |
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f0f1c0ac44
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Worked through all eight review comments. Seven were valid and are fixed; one is declined with evidence. Each thread has a reply and is resolved. Fixed: grouped scalar symbols ( Declined: the Two of the suggested fixes were deliberately not taken, with reasoning in the thread replies: lowercasing a One defect not raised in the review, found while verifying it: gosnmp emits OID names with a leading dot and profile OIDs carry none, so row index extraction and per-entry table tag joins never matched ( Net effect on collection: reachable symbols went from 7171 of 8225 declared to all 8225, and 1208 previously discarded device tag definitions across 181 profiles now reach the exported series. The PR description has been updated, since three items it previously listed as deliberately deferred are now fixed. |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6ef0465ab4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Second round: all seven addressed, each thread replied to and resolved. Also fixed the CI failure, which was my own test asserting on a DNS resolver's error text that differs between a developer machine and the runner; it now points the exporter at a loopback listener and asserts the connection arrives. Fixed: root context cancelled before shutdown waits ( Two of these were gaps in changes from the previous round rather than old behaviour, which is a fair catch: the grouped-scalar loop had no context check, and device identity moved to policy/host/port without One suggestion not taken. For One defect found while verifying yours, now fixed ( For |
|
@codex review |
|
Correction to my previous comment: the hextoint tag-column fix is |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fa771dd0e0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Third round: all five addressed, each thread replied to and resolved. Fixed: grouped symbols that are really table columns now collect as a table ( Two were gaps in fixes from the previous round, which is a fair hit rate against changes made under review. Chasing each one found more of the same class: the deadline audit turned up two further unchecked loops, including one where the check only looked present because it sat inside One suggestion declined, consistent with the earlier Two things I changed beyond the comments, both consequences of these fixes rather than new scope. Known and not fixed: Firepower's condition still does not filter. It names a metric tag column and compares a string, while the parser resolves symbol names and integers. Routing fixes row identity so the pools are distinguishable, but the filter needs the shared condition machinery extended, and the other five bundled Also flagged for elsewhere: the unconditional |
|
Correction: the once-per-profile condition fix is |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e6eb234ea1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Fourth round: all three addressed, replied to and resolved. Fixed: conditions comparing a textual column now apply ( One reverses a call I made last round. I declined the string condition as separate machinery not worth reshaping for one profile. The consequence given this time is what changed it: the filter being disabled means both high-capacity memory metrics were emitted for every pool, so the collector was exporting rows the profile did not ask for. That is a correctness problem, not a missing convenience. One I changed after the fix landed. The first pass rejected any policy whose retry ceiling reached The device identity has now been wrong twice, first missing the port on the exported attributes, now colliding within a policy. Rather than add a third test case, it is pinned structurally: identity rendering moved into one helper and a reflection test checks the key's fields against the exported attribute map in both directions, so adding a dimension to one side without the other fails the build. Stated rather than glossed: context cancellation is noticed at a retry-attempt boundary, not mid-read, so a walk already blocked in a receive waits out at most one remaining Also new and worth knowing: |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8cc6cdd043
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8cc6cdd043
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Sixth round: all seven addressed, replied to and resolved. Plus the CodeQL alert from the same batch, which is the one item on this PR dismissed rather than fixed: it flagged Fixed: empty-OID symbols skipped before walking ( On On scripted symbols, one honest cost. The UniFi case is unambiguous, a per-mille value carrying a Two fixes deliberately avoided a second source of truth. The v3 protocol validation delegates to the same switch the client resolves through, and its test names no expected set, instead running a mixed list through both the client and the validator and asserting agreement. That killed a mutant of "a hand-written list missing SHA512", which a fixed-set test would have shipped. Same reasoning as the earlier expansion guard, which no longer parses targets itself. One sibling defect closed alongside yours: the blank-host check trimmed before comparing but the stored value kept its whitespace, so |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a6a8220d32
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Seventh round: all three addressed, replied to and resolved. Fixed: policy request body bounded at 1 MiB with 413 ( The bulk-walk change is measured, not argued: against a real UDP SNMP agent, 5 columns by 200 rows, GETNEXT issues 1001 requests and GETBULK issues 41, so 24.4x fewer round trips, with the two result maps compared equal so identical values are demonstrated rather than claimed. Three things about it were read out of the gosnmp source rather than assumed, because two of them would have failed silently. SNMPv1 has no GETBULK and gosnmp does not fall back, it returns an error before sending, so bulk refuses to arm on a v1 handle. The collection context added earlier in this PR does govern the bulk path, since both walk functions are wrappers over the same send path. And The body limit is derived rather than picked: the documented sample policy is 602 bytes, the richest sample with every optional key set is ~3.7 KB, and a CIDR target covering the entire 65536-address cap is 30 bytes, so device count does not drive body size. 256 KiB was considered and rejected because a 20-policy by 50-target enumerated body already exceeds it. Worth saying plainly that this caps one body and not the process: gin has no concurrency limit here, so a rate limit is the next thing this endpoint wants if it is ever exposed beyond localhost. Flagged for elsewhere, not fixed here: |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 46a0f22781
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Eighth round: all three addressed, replied to and resolved. Fixed: the expansion budget is now policy-wide rather than per target ( The expansion guard has now been bypassed twice, both times for the same underlying reason. Rather than add a second, larger policy-wide number, the existing 65536 became the budget: one limit is one thing to reason about, and it is still counted through The timeouts are derived, and one deliberately diverges from the sibling. The metric-tag change proves its own scope rather than asserting it: the resolved column for all 4918 tags across the bundle was dumped before and after, and exactly 2 rows differ, the two Raritan tags going from unresolved to their OIDs. It routes through the same helper as the four tag features added in earlier rounds, so the next one will not miss it. One pre-existing issue surfaced, not changed here: the agent's remove-policy client timeout is 20s, below this server's ~24s DELETE ceiling, so a slow delete would be reported to the agent as failed while it actually completes. That is in the agent rather than this backend. |
A `tag:` renames the metric, so one table column can be declared twice and export under two names. The condition map was keyed by OID, so those two declarations shared one predicate: a conditional declaration filtered the rows of an unconditional one, two differing conditions left the last resolved one deciding both, and a condition the collector could not apply borrowed its sibling's. Conditions are now keyed by symbolDeclKey, the key poll state and retention already use, so the declaration that was polled is the one whose predicate is applied. No bundled profile declares one OID twice inside a single entry, so the change is visible only for an override. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…not merge The per-target key joined host, port, NetBox ID and SNMP context name into one string. Every one of those fields arrives over the API unrestricted, so a pair of values produces a single key: an ID of "a context=b" with no context name against an ID of "a" with a context name of "b". That key is now both the error map's key and the identity expandTargets collapses repeats on, so the collision drops a target the operator asked for and it is never polled, and the two targets share one error entry as well. Replace it with a comparable struct, which cannot collide by construction, and render it only for the status message. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…y name A batch POST that fails partway stopped the policies it had started by name. A DELETE and a POST for one of those names, both landing before the rollback, leave the name holding a runner the failed request never created, and the rollback stops that one: one refused request deletes an unrelated successful concurrent update. StartPolicyHandle now returns a handle to the runner it created, and StopPolicyHandle stops that runner only while it is still the one registered under its name. The comparison and the detach happen together under mu, so a replacement started between the two cannot be caught by it. Both rollback paths in the handler use it, and each attempts every handle rather than stopping at the first failure, so an abandoned request cannot leave a runner polling under a name no status report mentions. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The value switch, the conversion gate and the tag column path each listed the numeric types separately and none included Uinteger32, so a symbol a device answers with the standard unsigned application type fell through to the non-numeric error and was skipped. gosnmp decodes it as uint32, which always fits an int64. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…answer A symbol declaring `allow_duplicate: true` survives the contest for its metric name, which is how a profile offers alternative OIDs for one reading so a device answering either of them reports it. Nothing resolved them at collection time: when two such declarations answered in one poll, each appended a point carrying the same identity attributes, so the observable gauge received more than one value for a single OTLP series. The bundled dell/dell-powerconnect.yml reads CPU from a RADLAN OID and a DNOS OID this way. Upstream never emits both. It writes each reading into a map keyed on the metric name within a row, so the second write replaces the first and one value ships, though which one follows the map order it walked its OIDs in. The run's points now go through a sink that holds one observation per metric name and row, so a row cannot receive a second point. The declaration that keeps the row is chosen by the rule the name contest already uses, now a profiles.Precedence shared by both so they cannot drift: the profile's own declaration before an inherited one, then the longer OID, then the first declared. A device answering both therefore reports the same source on every poll and across restarts, and the reading kept brings its own status and display value attributes with it rather than mixing them with the other's. The row, not the full attribute set, is what a point is deduplicated on, since two readings of one row differ in exactly those two attributes. Retention reads the same key, so a carried point whose row a fresh point has taken is dropped as before, and one whose row another carried point fills is dropped as well. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
NoAuth and NoPriv are protocol names the client resolves, so an authNoPriv policy setting auth_protocol: NoAuth and an authPriv policy setting priv_protocol: NoPriv both passed validation. gosnmp checks the resolved protocols against the message flags on the way through Connect and rejects those pairs before dialing, so the API reported the policy as started while every collection failed. Validation now resolves both names and maps the level through the same helper NewClient dials with, and compares against the gosnmp sentinels the way gosnmp does. Its own check is unexported and reachable only by dialing, so the pairing is pinned by a test that drives every security level against every protocol name through Connect and requires the validator to agree. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
tripplite/tripplite-ups.yml writes convert: to_one on tlUpsTestDate and tlUpsTestResultsDetail. Only conversion was decoded, so both symbols declared no conversion, their OctetString readings reached the value conversion with nothing to apply, and both metrics were dropped as non-numeric. Symbol now reads the alias when the documented spelling is absent, the same way Profile reads sysObjectID. The two spellings are peers on one node rather than a general and a specific declaration, so the documented spelling wins when a symbol carries both, and that is pinned. Upstream ktranslate decodes only conversion as well, so this is a defect in the profile that we accommodate rather than a field we were failing to read. The vendored tree stays byte-identical to upstream, so the fix is in the decoder. The alias appears twice in the whole bundled set, both at symbol level in that one file and none at tag or column level. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The listen address was formatted as host:port, so --host ::1 produced ::1:8078 and --host :: produced :::8078. net.Listen refuses both with "too many colons in address", which left the API unable to bind IPv6 at all. net.JoinHostPort brackets a host that carries colons and leaves hostnames, IPv4 literals and an empty host exactly as they were. The README now shows the IPv6 spelling beside the 0.0.0.0 one. The only other host-and-port string in the module is targetKey.String, which renders a status error message that nothing parses back, so it is left alone. The SNMP client passes the host and the port to gosnmp separately and gosnmp joins them the same way. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
gosnmp asks for the passphrase that goes with a protocol whenever the protocol resolves above the sentinel, whatever the security level asks for, while validation asked only at the level that needs one. So noAuthNoPriv with auth_protocol: SHA and no passphrase, noAuthNoPriv with priv_protocol: AES and no passphrase, and authNoPriv carrying a priv_protocol with no priv passphrase all passed validation and then failed before dialing, which is the same defect as a sentinel protocol at a level that needs a real one. The condition is a property of the resolved protocol rather than of the level, so it goes where the level and protocol check already lives, which now takes the whole authentication block and is named for the gosnmp method it corresponds to. The reverse direction is matched too: a passphrase beside a sentinel protocol is never read by gosnmp, so it is not a reason to reject a policy. The passphrase presence dimension is a sibling table on the same oracle rather than a fourth loop in the existing one, so a passphrase failure is not reported as a protocol name failure and the level and protocol table keeps supplying both passphrases to isolate its own verdict. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
test-coverage piped go test -json through the cmd filter into tparse, so the filter dropped the cmd failure events and the pipeline reported the filter's own zero status. CI runs only test-coverage, so a failing test under cmd left the workflow green. Write the events to a file, keep go test's status in a variable and exit with it, then filter the file for the report and the coverage profile. Filtering the artifacts instead of the test invocation keeps the status explicit and needs no shell option, which make would otherwise have to carry through SHELL and .SHELLFLAGS since each recipe line gets a fresh shell. The recipe is inherited from the orb-discovery backends, where the hole is latent because none of them has tests under cmd. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
pollState and the device store were keyed by device identity alone. A device replaced at an address that now matches a different profile shares one symbolDeclKey with the old profile for every declaration the two have in common, so the new profile was throttled against the old device's poll window and retention carried the old device's point into the new run. Inherited IF-MIB metrics went stale for a poll period, a long poll_time_sec declaration for much longer. Record the profile each device matched and forget the device's observations and poll timestamps when the match changes, which is the same drop a failed run and a profile that stops matching already take. Keeping per-profile state instead would spare a device flapping between two profiles a re-poll, at the price of state with no reader and no bound; the re-poll costs one collection cycle and cannot leak. Profiles are identified by relative path, falling back to the base name, so two profiles sharing a base name in different directories are told apart. reviewProfile already named them that way and now shares the helper. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…string The report and coverage filters matched "cmd" anywhere on a line, so a package path or test output containing it would have been dropped from both. No line in the current suite does, so this closes a latent hazard rather than a live one. Anchoring them to the package makes the filters say what they mean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A table entry can declare one column twice under one exported name, one poll period and two conditions, which is what an override leaves behind when it keeps the second symbol with allow_duplicate. Both declarations then have one symbolDeclKey, so the map holding their resolved predicates kept only the one resolved last and applied it to both. Rows selected by the other predicate were never exported. The predicate now travels on the state of the symbol that declared it, and the collection phase reads it back off that same state, so no key decides which declaration a condition reaches. The declaration key keeps the granularity the poll window and retention want: two symbols on one column, name and period are one walk and one window, and the condition filters rows the walk already returned.
gosnmp logs every packet it sends at debug level and its packet format prints the community verbatim, then prints again the community it parses out of each reply. A debug run therefore wrote the credential of every v1 and v2c session to the process log in plaintext. SlogAdapter now removes the community from every line it forwards. The match is on the value rather than on the library's format string, so a gosnmp release that renames or moves the field cannot reopen it, and it covers the reply path as well as the send path. The v3 path is unaffected: the USM safe string prints the user name and the per-packet authentication and privacy parameters, which are an HMAC and an IV, and neither passphrase. A test pins that. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ads it The condition row cache was keyed by column OID and held the rendered string, built with whichever condition resolved first. Two conditions can name one column through different renderings, a sibling symbol's raw reading and a tag column's enum being the shape, so the second compared against the first one's rendering and matched nothing. A column read as raw "1" by one condition and as "up" by another emitted rows for one of them only, whichever the profile happened to declare first. The cache now holds the PDU and each condition renders the row it reads. The key stays the column OID, so two conditions naming one column still walk it once, and the cache cannot go stale the way a key enumerating every rendering input would. No bundled profile declares this shape: across the 205 profiles, five columns carry two conditions each and all ten agree on their rendering. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
gosnmp reports a malformed reply by quoting the bytes it could not parse, and the community is an octet string near the front of every v1 and v2c message, so a parse error carries the credential. A reply whose community length octet is corrupt yields "not enough data for OctetString (29495 vs 64): 0482<community><rest of packet>", and one cut short inside the community yields a prefix of it. The log adapter does not reach these. The error is returned, not logged by the library: the collector logs it at warn level, and the policy status endpoint serves it as last_error, so it disclosed the community at any log level and to an audience wider than the process log. Client.Walk now redacts the error, which is the one boundary holding both the community and the error. Lowercase hex is the only encoding to remove, since gosnmp formats packet bytes with %x and with nothing else, and leaving the words of the message unmatched is what keeps a short community from mangling the diagnostic. Every prefix is removed, not only the whole value, because a truncated dump carries a prefix and a prefix of a credential is still one. What survives is the parse stage, the byte counts and the rest of the dump. Pinned against the real library: a fake agent rewrites its replies, so the errors under test are gosnmp's own, taken from a real packet. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…eries Profile tags, device and row alike, were appended after the collector's identity attributes. A duplicate attribute key resolves last-value-wins, so a profile declaring a tag named policy, device_ip, device_port, netbox_id, snmp_context or row_index replaced the real value with its own, and two policies polling one endpoint could export series nothing tells apart. A tag taking one of those names is now dropped and reported once per profile, rather than the profile being refused: a bundled profile is vendored and cannot be edited, so refusing to load one would take away every metric it carries over one attribute name. The reserved set is read back from appendIdentityAttrs, so a dimension added to the identity reserves its name by the same edit, and the reflection test that pins the device key against the exported attribute map pins the set with it. No bundled profile declares such a tag, at either level, so this reaches only an override. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
metrics_interval and snmp_timeout are multiplied by time.Second. A large positive value overflows int64 and wraps to a fraction of a second, so an interval of 40423014371506394 became about a microsecond. Validation and the timeout-versus-interval comparison then read the wrapped values and passed, and the policy scheduled a near-continuous collection job that any client reaching the policy API could ask for. Both are now capped at one year of seconds before the conversion, in validatePolicy and again in NewRunner, which is where the multiply happens and is reachable without the API. One year rather than the representable ceiling of about 292 years: no collection interval reaches it, and keeping both durations that far inside the range also keeps snmp_timeout times the capped attempt count from wrapping. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…icy path The root is an operator flag and may be spelled relative to the working directory. filepath.Rel cannot relate a relative root to an absolute candidate, so with --snmp-profiles-root=profiles a policy naming the documented absolute form under that root was rejected. The root is made absolute before either containment check, so both compare paths of one kind. It sits after the ".." substring test, which stays the first thing the policy-supplied value meets and still runs before any filesystem access, and it reads only the working directory rather than the value itself. The canonical comparison still resolves symlinks on both sides. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
6331412 to
7a22b9a
Compare
…tion -otel-export-period is multiplied by time.Second. A large positive value overflows int64 and wraps to a fraction of a second, so 40423014371506394 became about a microsecond: the non-positive check passed on the number the operator typed, and the reader was then handed the wrapped one and exported near-continuously under a startup line reporting the huge period. The bound is the one the policy fields already use rather than a second number. It moves to config as MaxDurationSeconds, with the reasoning for a year over the representable ceiling of about 292 years stated once beside it, and policy reads it back. The check sits in SetupMetricsExport, where the multiply happens, matching where the policy fields are bounded and leaving the flag with nothing to restate; being operator-supplied rather than reachable from the policy API changes how it gets there, not that a wrapped duration is wrong. An empty endpoint still disables metrics without inspecting the period, and a non-positive one is still refused. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A reading appends attributes of its own after the profile's tags: the enum member its value falls on, under the symbol's export name plus _status, and the text a conversion rendered it as, plus _value. A duplicate attribute key resolves last-value-wins, so a profile declaring a tag named CPU_status beside a symbol exported as CPU with an enum had that tag replaced on every point the symbol produced, with nothing said. The previous round saw this and left it, on the grounds that the row key is taken before the pair is appended so row identity survives. That was too narrow. A declared tag disappearing without a word is the failure this module refuses in the other direction, and the same row key is the reason the tag is what stays: the tag is a declaration carrying a reading of another column that appears nowhere else on the point and is already part of that key, while the derived attribute is generated and renders the value the point exports under its own metric. The derived attribute is dropped and the tag kept. The pair is not reserved as a static list, which the previous round rightly objected to: it is one pair per symbol, and each half exists only if the symbol declares the thing that fills it. What is knowable from the profile is the name, since only an enum can produce a label and only to_one, hextoip, hwaddr or regexp render text; only whether a given reading fills it needs a device answer. So the collision is named once per profile by reviewProfile, over the entry's own symbols, and the drop itself is a presence check where the attribute is appended. Device tags are checked against every symbol, row tags against the entry that applies them. No bundled profile collides: 1641 derived names against 1208 device-level and 3708 row-level tag names, so this reaches only an override. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Twenty-ninth round: both addressed, replied to and resolved. The branch has also been rebased onto develop, picking up #565, #566 and #567 with no conflicts. Fixed: the OTLP export period is bounded before it becomes a duration ( The bound is shared rather than chosen twice. The one-year limit moved into The second finding reverses a call made one round earlier, and correctly. That round fixed profile tags overwriting identity attributes, considered this mirror case, and called it benign because row identity survives. A declared tag vanishing without a word is the same failure mode this module has refused everywhere else. What is dropped went the other way this time, deliberately: the derived attribute is dropped and the profile tag kept. The tag carries a reading of another column that exists nowhere else on the point, while No static reserved list, which the earlier round objected to for a reason that still holds. The work is split by what each site can know: enforcement is a presence check at the append site, since only a device answer says whether either half is filled, while the report is once per profile, since the names are decidable from the declared enum and conversion alone. Zero bundled collisions at both levels. The device-level and row-level tag counts match the previous round's independent scan exactly, which is a useful cross-check that both walks see the same declarations.
|
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1dd1f06f30
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
gosnmp starts every request with a slice of retries+1 request IDs, so the policy field is an allocation size the caller chooses. A few billion is a valid capacity of several gigabytes and exhausts the backend on the first scheduled collection. The existing attempt cap does not cover it: it clamps a local count so the retry ceiling multiplication cannot overflow, and never changes the value handed to the client. Bound the field itself at ten, on both the API path and in NewRunner, the way the seconds bounds are enforced. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Declarations agreeing on metric name, OID and poll period share a poll window but took a walk each. For an allow_duplicate override whose declarations differ only by condition, a first walk that answered started the window for both, and a second that failed left its rows absent for the whole poll_time_sec because the next cycle found that window still open. Cache each run's walks by OID so every declaration naming it reads the same answer, error included: one walk cannot disagree with itself, and a column the device has already failed to answer is retried on the next cycle rather than inside this run. The cache also removes the second request, which is the waste the shared window key was introduced to avoid: across the bundled set it drops 635 repeated walks, and it holds at most one full-table walk since no bundled profile roots two entries at one table. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Thirtieth round: both addressed, replied to and resolved. Fixed: the retries a policy hands to the SNMP client are bounded ( A correction on the first, since the reported mechanism is not the one that bites. gosnmp does allocate The ceiling is ten, bracketed by gosnmp's own default of three and net-snmp's five, and past what an interval can spend given each attempt costs a full The second is the concrete failure behind a deferral. An earlier round argued walk sharing was an optimisation and the shared declaration key was its precondition. The precondition existed; this is what the deferral left open. Caching was chosen over tracking success separately, because tracking only shortens the outage from a full poll period to one cycle. One detail decided the shape: caching only successes does not fix it, since the second declaration would still issue its own failing walk, so the cache holds one outcome per OID per run including the error. That round's own estimate of the work held exactly, about 40 lines and one parameter on three signatures, with no change to poll state, retention or precedence. Since five rounds have now built on this state and three defects here came from one fix weakening another, each of the five behaviours was rechecked by running its tests and mutating the code it guards to confirm the tests still kill it. A side effect worth knowing: no bundled profile can reach the finding, but forty of them repeat an OID across entries, so 635 repeated walks per run are now removed. The one place this collects less than before is that a failed first read of a repeated OID is now shared rather than retried independently, which is pinned by a test. |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7236f1a9de
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| .PHONY: build | ||
| build: | ||
| CGO_ENABLED=$(CGO_ENABLED) GOOS=$(GOOS) GOARCH=$(GOARCH) GOARM=$(GOARM) go build -mod=mod -o ${BUILD_DIR}/snmp-telemetry cmd/main.go |
There was a problem hiding this comment.
After running the repository's make work, invoking this backend's make build directly fails before compilation because the generated go.work is discovered and Go rejects -mod=mod in workspace mode. Export GOWORK=off from this Makefile or set it on the build command so the backend's advertised make target remains usable with the repository workspace enabled.
AGENTS.md reference: AGENTS.md:L47-L52
Useful? React with 👍 / 👎.
2 participants
What this does
orb-telemetry/snmp-telemetry/existed as ~230 untracked files, written locally startingfrom a copy of
snmp-discovery. It had never been committed, built by CI, linted,released, or reviewed. This PR makes it a first-class, tested, releasable module. It does
not make the agent run it; that is the follow-up.
It is also the first backend outside
orb-discovery/, so a fair amount of this PR isteaching the repo plumbing that a second parent directory exists.
Why the profile change is a fix, not packaging taste
Profiles were loaded by walking a directory resolved through three fallbacks, ending at a
hardcoded
/usr/local/share/snmp-profiles. No image stage creates that path, and every Gobackend copies only its built binary. So with nothing configured,
os.Statfailed andevery policy failed to start. Profiles are now embedded with
//go:embed all:snmp-profilesand an optional override directory, following
gnmi-discovery/mapping. Without this, thefollow-up PR would ship a backend that cannot start.
Changes
orb-discovery/snmp-telemetrytoorb-agent/orb-telemetry/snmp-telemetry.gained a
parent-dirinput defaulting toorb-discovery, so all six existing callersare untouched. This also unblocks
opentelemetry-infinitylater.context_name: config field, client wiring, and validation rejecting it forv1/v2c, which have no context concept.
kentik/snmp-profilesmain. Files are verbatim, upstream has noNOTICE, and their Apache 2.0 LICENSE is vendored alongside. Provenance recorded in
profiles/PROVENANCE.md.orb-telemetry/README.mdindex and a backend README.Expect one red check
pr-title-lintruns onpull_request_target, so GitHub evaluates the workflow from thebase branch. This PR adds
snmp-telemetryto the scope allowlist, but develop's copy doesnot have it yet, so the title check reads the old list and fails on this PR's own title.
It resolves itself once this merges. Branch protection does not currently mark the check
required, so it reports status without blocking. If you would rather see it green, the
allowlist change can go in ahead of this as its own PR under an existing scope.
How this was reviewed
Five adversarial reviewers went over the branch across independent dimensions, then
seventeen rounds of automated review, each round replied to and resolved. That is 63
findings. Most were valid; several were refuted with evidence and a few were answered by
doing something other than what was suggested. Every fix carries a regression test that was
checked by reverting the fix and confirming the test fails, and every commit builds and
tests green on its own.
The branch was also run against real device recordings, see Validation below.
What the review changed
Grouping the substantive ones rather than listing all 63.
Collection was quietly collecting far less than the profiles declare. 12.8% of declared
symbols were unreachable because grouped
symbols:entries with notable:had no dispatcharm, and five profiles collected nothing at all. Profile-level
metric_tagswere parsed anddropped, discarding 1208 tag definitions across 181 profiles, so every exported series was
missing
SysName,SysDescrand friends. A leading-dot mismatch between gosnmp's PDU namesand profile OIDs broke row index extraction and per-entry tag joins, and was invisible
because the test doubles encoded the same bug.
Several things were decided by Go map iteration order, so identical device data could
produce different output between polls or between restarts: a misplaced profile override won
a coin flip for a sysObjectID, duplicate enum values returned different labels, and a
symbol's row identity depended on how many rows a device happened to answer with.
A device was identified by host alone, so two policies naming one device overwrote each
other and cross-contaminated throttled values. Nothing ever forgot a device, so a deleted
policy or a dead device kept exporting last known values for the life of the process.
The API accepted policies that could never work and reported them as running: no
targets, blank or malformed target hosts, unsupported SNMPv3 protocol names, a v3 policy
with no username, and a policy name that no route could address, where a trailing slash
silently deleted a different policy.
Security. Policy creation could resolve any process environment variable and send it as
an SNMP community to an attacker-controlled destination;
profiles_dirwas unconfined andlater escapable through a symlink; the request body was unbounded; the server had no read
timeouts; and the listener defaulted to every interface while the agent only ever connects
over loopback.
Repo plumbing did not know a backend could live outside
orb-discovery/. The PR couldnot pass its own title check, a backend-scoped commit would have cut a spurious agent
release, and
make lint-allsilently skipped a module CI blocks on.A symbol's tag renames its metric
Adopted deliberately after a lab run surfaced an exported attribute keyed literally
tagand an investigation of ktranslate established what the field actually means. Upstream's
Mib.GetName()returns the tag in preference to the name, and that is what becomes themetric name; the attribute this collector was emitting has no upstream counterpart.
This is the largest behaviour change in the PR and it is breaking for consumers: 954
declarations change metric name, 230 names disappear, 85 appear, and the
tagattribute isgone. Upstream's duplicate resolution came with it, implemented from its source rather than
approximated: a symbol declared in the profile beats one inherited from an extended profile,
then the longer OID wins, and
allow_duplicateis never dropped.42 symbols across 15 profiles are dropped by that rule. 28 of them lose to an identical
OID, so only 14 are real losses, and 12 of those are the intended CPU and memory overrides
the profiles exist to express. Seven more would have lost and are kept by
allow_duplicate.The full table, with the winner and the deciding rule for each, is in the review thread.
Two upstream behaviours could not be modelled exactly and both were measured inert on the
bundled set: upstream contests device and interface metrics in separate namespaces where this
collector has one, and it leaves an exact tie to map order where declaration order decides
here.
Validation against real devices
Built from a clean export of the pushed head and run against the four LibreNMS-recording SNMP
simulators in orb-test-lab with an OTLP collector attached. 11,672 data points across 39
metrics, no collection errors, all four devices matched a profile. Confirmed on real device
data rather than fixtures: profile-level tags present on every point, full device identity,
row_indexon table rows, a barehost:portOTLP endpoint connecting, and GETBULKcompleting a walk in 2 requests where GETNEXT needs 1001. Deleting the policy produced zero
further data points over the following 25 seconds. The only warnings in the whole run were
the ones this branch deliberately emits, matching their reported counts exactly.
Known and deliberate
since the tree must stay byte-identical to upstream:
xerox/workcentre.ymldeclaresifInDiscardsat theifInUnknownProtosOID and upstream's rule keeps it over the correctinherited one, and
_general/if32-mib.ymlgives 32-bit counters the 64-bit names, so ahigh-capacity series can now be fed by a counter that wraps sooner. Both want upstream
issues.
profile: an unimplemented
powerset_statusconversion, scripted symbols whose transformthis collector does not run, trap and table entries in the Ruckus profile's alternate
schema, and a
sysobjectidthat cannot be indexed.profiles_diris narrowed, not closed: the resolution is notrepeated at use time, so someone who can write inside the root can still swap a component.
duplicate resolution is settled by declaration order rather than upstream's map order.
Verification
GOWORK=off go test -raceclean across the module;go vetclean;make lintreports0 issues;
make test-coveragewrites exactly one line.line ending differences, and the LICENSE matches by sha256.
by reverting each fix and confirming the test fails.
Follow-up
Agent backend, port map entry, Dockerfile stage, version threading, default config entry,
and
docs/backends/snmp_telemetry.md.Two flags added by this PR have to be threaded through when the agent starts launching this
backend, since without them a policy cannot resolve an environment variable or set an
override directory at all:
--policy-env-varsand--snmp-profiles-root.Noted, not fixed here
.github/semantic-release/backend.releaserc.jsonhas no"preset": "conventionalcommits",so for every backend a
feat(x)!: ...title resolves to no release at all, whilepr-title-lint accepts that title. Only the
BREAKING CHANGE:footer cuts a major. One linefixes it, but it changes release behaviour for all existing backends.
orb-discovery/snmp-discoverycarries several of the same defects fixed here: anunrestricted policy environment-variable resolver, a listener defaulting to every interface,
an unbounded request body, an exact-string Content-Type check, and the unconditional
WithInsecure()on its OTLP exporter that the gNMI and network backends share.