Fix telemetry from the store. Clean up metadata count telemetry. #2177
Conversation
…ce that's the standard order of precedence. Also allow the flag to have the form '(--testnet|-t)=(true|false)'. And overhaul the comment on it with more details.
…get telemetry from the deeper stuff.
…nything, and no one noticed.
…tance of a flag should take precidence. Unit tests on isTestnetFlagSet.
… tests on getTelemetryGlobalLabels as well as a canary for if the SDK tries to break it.
WalkthroughThe changes in this pull request primarily focus on the removal of non-functional telemetry components from the metadata module, which simplifies its structure. Additionally, the telemetry system is fixed to restore functionality that was lost in version 1.19. Enhancements to command-line argument handling and telemetry configuration are also introduced, along with new tests to validate these functionalities. The logging capabilities are extended with a new debug logger, and several methods in the keeper package are modified to streamline event handling and validation processes. Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
internal/logger.go (2)
34-40: LGTM! Consider a minor comment improvement.The new
NewBufferedDebugLoggerfunction is well-implemented and consistent with the existing logging functions. It correctly creates a debug-level logger using theNewBufferedLoggerfunction.Consider updating the comments to match the style of
NewBufferedInfoLogger:// NewBufferedDebugLogger creates a new logger with level debug that writes to the provided buffer. // Error log lines will start with "ERR ". // Info log lines will start with "INF ". -// Debug log lines will start with "DBG ". +// Debug log lines are included and will start with "DBG ".This change would emphasize that debug logs are included, contrasting with the info logger where they are omitted.
34-40: Positive enhancement to logging capabilitiesThe addition of
NewBufferedDebugLoggeris a valuable enhancement to the logging infrastructure. It expands the available logging options while maintaining consistency with existing functions. This change aligns well with the PR objectives of improving telemetry and logging capabilities.Consider documenting this new logging option in any relevant developer guides or README files to ensure that other developers are aware of this new capability for debug-level logging.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (11)
- .changelog/unreleased/bug-fixes/2144-remove-metadata-count-telemetry.md (1 hunks)
- .changelog/unreleased/bug-fixes/2177-add-store-telemetry.md (1 hunks)
- cmd/provenanced/cmd/root.go (4 hunks)
- cmd/provenanced/cmd/root_test.go (2 hunks)
- internal/logger.go (1 hunks)
- x/metadata/keeper/objectstore.go (0 hunks)
- x/metadata/keeper/record.go (0 hunks)
- x/metadata/keeper/scope.go (0 hunks)
- x/metadata/keeper/session.go (0 hunks)
- x/metadata/keeper/specification.go (0 hunks)
- x/metadata/types/events.go (0 hunks)
💤 Files with no reviewable changes (6)
- x/metadata/keeper/objectstore.go
- x/metadata/keeper/record.go
- x/metadata/keeper/scope.go
- x/metadata/keeper/session.go
- x/metadata/keeper/specification.go
- x/metadata/types/events.go
✅ Files skipped from review due to trivial changes (1)
- .changelog/unreleased/bug-fixes/2177-add-store-telemetry.md
🧰 Additional context used
🪛 LanguageTool
.changelog/unreleased/bug-fixes/2144-remove-metadata-count-telemetry.md
[uncategorized] ~1-~1: Possible missing comma found.
Context: ...he telemetry counters from the metadata module since it wasn't actually doing anything...(AI_HYDRA_LEO_MISSING_COMMA)
🔇 Additional comments (8)
cmd/provenanced/cmd/root.go (5)
16-16: Importingviperfor configuration managementThe addition of
"github.com/spf13/viper"is appropriate and ensures that configuration settings are handled effectively.
23-23: Addingmetricspackage for telemetry supportIncluding
"cosmossdk.io/store/metrics"allows the application to set up store metrics, enhancing telemetry functionality.
310-310: Integrating store metrics with telemetry configurationThe use of
setStoreMetrics(getTelemetryGlobalLabels(logger, appOpts))appropriately configures store metrics based on telemetry settings, enhancing observability.
474-481: Properly setting store metrics when telemetry is enabledThe
setStoreMetricsfunction correctly configures the base application to set store metrics when telemetry is enabled, ensuring metrics are captured as intended.
493-533: Retrieving telemetry configuration from application options effectivelyThe
getTelemetryGlobalLabelsfunction efficiently extracts telemetry settings fromappOpts, accommodating both cases whereappOptsis a*viper.Viperinstance and when it is not. This ensures that telemetry configurations are accurately applied.cmd/provenanced/cmd/root_test.go (3)
340-476: Well-Structured Tests forisTestnetFlagSetFunctionThe added test cases comprehensively cover various combinations of environment variables and command-line arguments, ensuring robust validation of the
isTestnetFlagSetfunction's behavior.
478-688: Comprehensive Testing of Telemetry ConfigurationThe
TestGetTelemetryGlobalLabelsfunction effectively tests multiple scenarios for telemetry settings using both Viper and custom options. This ensures that the telemetry global labels and enabled states are correctly handled across different configurations.
690-710: Proactive Canary Test for Telemetry Keys IntegrityThe
TestTelemetryKeysCanaryfunction serves as a safeguard against future changes in the SDK that might affect telemetry functionality. This proactive approach helps maintain the integrity of telemetry configuration keys and types.
Description
closes: #2144
This PR does the following:
isTestnetFlagSetto make it more accurately check for stuff.Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.
docs/) or specification (x/<module>/spec/).godoccomments..changelog/unreleased(see Adding Changes).Files changedin the Github PR explorer.Codecov Reportin the comment section below once CI passes.Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests
Chores