Fix broken metrics part 1 #31
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…, fix typos and make various improvements to the config file text and structure.
When run without --config the embedded copy of etc/rotonda.conf is used instead, and targets.proxy is removed if --proxy-destination is not supplied on the command line. If --bgp-listen or --bmp-listen are supplied on the command line they override the BMP and BGP listen settings in the embedded config. - Allow BGP peers to be missing.
- Add missing semi-colon. - Filter out not in.
…nnections on a particular socket.
…age can link to the actual RIB query HTTP API rather than incorrectly assume a fixed path of /prefixes/.
…e-prefixes-query-link
…ined in roto scripts loaded from a roto script directory. - Refactors roto script file reading from the units/targets to the Manager. - Checks script loading, compilation and valid filter names at config load time instead of at filter execution time. - Add MVP behaviour tests (WIP). - Introduces a FilterName type.
…in case I return to this later.
…e required .roto files, e.g. if running from `cargo install`. Cleanup config initialisation and standard variable naming.
…ts_dir` dir `etc` doesn't exist. This change had several knock on effects: - Introduced RotoError::LoadError. - Upgrade clap crate to fix bug with clap switcing help on own line or not even when disabled. - Upgrade toml crate to fix inconsistent line breaks in diagnostic dump of post-processed config TOML. - Adjust position handling in `Marked` due to changes in serde_spanned pulled in by other crate upgrades. - Added new cmd line arg --print-config-and-exit and introduced `Terminated` for use in cases where `ExitError` wrongly limits us to just an error exit rather than a normal exit. - Various minor tweaks to the clap config for improved read and usability. - Log when exiting.
…using a mock BGP `Session`.
… whose APIs are linked to.
ximon18
changed the base branch from
blocks-wip
to
fix-broken-bmp-routers-html-links
…ix-broken-metrics
Base automatically changed from
fix-broken-bmp-routers-html-links
to
blocks-wip
October 12, 2023 07:55
…ater results may be different if called again.
… the BMP TCP input receiver on non-fatal errors (otherwise loss of the receiver causes the router read loop to abort anyway).
density215
reviewed
Oct 19, 2023
density215
pushed a commit
that referenced
this pull request
Dec 8, 2023
* Add a special metric output format for use by unit tests, and a callback fired on each VM exec that rejects the input (to enable easy counting of filtered out input messages), and remove support for metric types we don't need. Use the new callback to invoke the till now unused message_filtered() status reporter fn. * Extend filter unit tests to show that the filtered message count metric now works. * Remove the bmp_in_connection_count metric as connections are handled by bmp_tcp_in while this metric is based on BMP initiation messages received, so this is (a) misleading and (b) doesn't work anyway as when the sys name changes so can the router id and thus the changed metric would be in a different metric set, there'd be no effect on the previous metric. * Follow changes in router id caused by a new sysName received by a subsequent BMP Initiation Message. * Make test metrics easier to query, and add initial tests for the one and only remaining bmp-in metric. TODO: add a test showing bmp_in_num_invalid_bmp_messages increasing. * Remove the footgun of having to initialiize metrics per router, just ensure they are initialized on first use, otherwise metrics get lost. * Add a test showing that invalid BMP messages cause the bmp_in_num_invalid_bmp_messages counter to increase. * Test the custom allocator metrics. * Refactor bgp_tcp_in to be testable and add first metric test. * Sort test metric output for easier reading when inspecting the contents manually during development. * Metric name typo correction. * Mock the TcpStream as well so that we can accept a simulated connection to test the `bgp_tcp_in_connection_accepted_count` metric. * Also test the connection lost and disconnected bgp-tcp-in metrics by using a mock BGP `Session`. * Improved naming of RIB merge update statistic metrics. * Introduce the concept of fatal vs non-fatal I/O errors and don't drop the BMP TCP input receiver on non-fatal errors (otherwise loss of the receiver causes the router read loop to abort anyway). * FIX: Don't abort BMP TCP input reading on non-fatal I/O errors. * Add a test for some of the bmp-tcp-in metrics. * Import SeqCst directly to be consistent with other usages and to be less verbose.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR replaces accidentally closed PR #27.
There will be a "part 2" soon, but for now this PR is stopping here.
Add unit or integration tests (as relevant) for all metrics showing that they update as expected.
Status:
BgpTcpInMetrics: ✅ (incomplete)BmpInMetrics: ✅BmpMetrics: TODOBmpProxyMetrics: TODOBmpTcpInMetrics: TODOGateMetrics: TODOMqttMetrics: TODORibUnitMetrics: TODORibMergeUpdateStatistics: TODORotoFilterMetrics: ✅TokioTaskMetrics: TODOTrackingAllocator: ✅This PR fixes the following issues:
bgp_tcp_in_diconnect_count.And this PR also:
Target::Testfor obtaining metric snapshots in a form more conducive to testing (only compiled in test mode, not production mode).fn AnyStatusReporter::metrics()for easier access to metrics for testing.Filterable::filter()to take afiltered_fn(SourceId)callback to make it easier to know when payloads are filtered out, especially when filtering on a collection of payloads rather than a single payload.// TEST STATUS: [ ] makes sense? [ ] passes tests?style comment to track which metrics have been assessed as to whether they actually make sense or not and whether or not they are covered by tests that pass.13/78
TEST STATUScomments are "green", so 65/78 remain to do, although not all are equally important.