-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
feat(telemetry): enable statsd
and dogstatsd
telemetry sinks
#18646
Conversation
statd
and dogstatsd
telemetry sinksstatsd
and dogstatsd
telemetry sinks
WalkthroughWalkthroughThe changes involve enhancing the telemetry and metrics system to support different backends, specifically statsd and dogstatsd, in addition to the existing in-memory sink. New configuration options have been added to specify the metrics backend type, statsd server address, and Datadog hostname. The code now handles the initialization of the specified metrics sink and integrates it into the telemetry system. Additionally, there are updates to tests and configuration defaults to align with these new features. Changes
TipsChat with CodeRabbit Bot (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 3
Configuration used: CodeRabbit UI
Files ignored due to filter (4)
- go.mod
- go.sum
- simapp/go.mod
- simapp/go.sum
Files selected for processing (1)
- telemetry/metrics.go (6 hunks)
Additional comments: 1
telemetry/metrics.go (1)
- 206-206: Verify that the
DisplayMetrics
method implementations can handlenil
values forhttp.ResponseWriter
andhttp.Request
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- server/config/toml.go (1 hunks)
Additional comments: 2
server/config/toml.go (2)
121-133: The configuration template has been correctly updated to include the new telemetry sink options for
statsd
anddogstatsd
. This aligns with the PR objectives and the AI-generated summaries.121-133: Verify that the new configuration options are accompanied by updates to the documentation and that there are tests covering these new features.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
This reverts commit 52d7b96.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK. Thanks!!! Left a minor comment. Also, let's get a small changelog entry in for the new support.
@alexanderbez addressed nits |
@alexanderbez are you cool to backport into |
Yeah, I don't see why not. This change is simple and purely additive. No objections from me. @tac0turtle? |
Co-authored-by: Aleksandr Bezobchuk <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (3)
- CHANGELOG.md (1 hunks)
- server/config/toml.go (1 hunks)
- telemetry/metrics.go (8 hunks)
Files skipped from review as they are similar to previous changes (1)
- server/config/toml.go
Additional comments: 2
CHANGELOG.md (1)
- 59-59: The changes in the
CHANGELOG.md
correctly reflect the updates made to the system, including the addition of telemetry features, updates tosdk.DecCoins
, improvements to thex/gov
module, and enhancements to theclient.Context{}
. The links to pull requests provide traceability for the changes.telemetry/metrics.go (1)
- 207-213: Please verify that passing
nil
for bothhttp.ResponseWriter
and*http.Request
toDisplayMetrics
is safe and will not cause any issues in all implementations ofDisplayableSink
.
@julienrbrt updated v0.50 app toml as well |
Can you run |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files ignored due to filter (42)
- client/v2/go.mod
- client/v2/go.sum
- tests/go.mod
- tests/go.sum
- tests/starship/tests/go.mod
- tests/starship/tests/go.sum
- tools/confix/data/v0.50-app.toml
- tools/confix/data/v0.51-app.toml
- x/accounts/go.mod
- x/accounts/go.sum
- x/auth/go.mod
- x/auth/go.sum
- x/authz/go.mod
- x/authz/go.sum
- x/bank/go.mod
- x/bank/go.sum
- x/circuit/go.mod
- x/circuit/go.sum
- x/distribution/go.mod
- x/distribution/go.sum
- x/evidence/go.mod
- x/evidence/go.sum
- x/feegrant/go.mod
- x/feegrant/go.sum
- x/gov/go.mod
- x/gov/go.sum
- x/group/go.mod
- x/group/go.sum
- x/mint/go.mod
- x/mint/go.sum
- x/nft/go.mod
- x/nft/go.sum
- x/params/go.mod
- x/params/go.sum
- x/protocolpool/go.mod
- x/protocolpool/go.sum
- x/slashing/go.mod
- x/slashing/go.sum
- x/staking/go.mod
- x/staking/go.sum
- x/upgrade/go.mod
- x/upgrade/go.sum
Files selected for processing (1)
- telemetry/metrics.go (8 hunks)
Additional comments: 9
telemetry/metrics.go (9)
4-14: The addition of the
net/http
import aligns with the introduction of theDisplayableSink
interface, which useshttp.ResponseWriter
andhttp.Request
.23-30: The new constants for different metric sink types (
MetricSinkInMem
,MetricSinkStatsd
,MetricSinkDogsStatsd
) are added to support the configuration of various telemetry sinks.32-35: The
DisplayableSink
interface is introduced to define a method for displaying metrics, which is a part of the telemetry system refactoring to support different metric sinks.63-79: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [37-77]
The
Config
struct is expanded with new fields (MetricsSink
,StatsdAddr
,DatadogHostname
) to configure the metrics backend type and the addresses forstatsd
anddogstatsd
.
82-87: The
Metrics
struct has been refactored to include a genericsink
field of typemetrics.MetricSink
, replacing the specificmemSink
. This change supports the use of different metric sinks.103-141: The
New
function is modified to support the instantiation of different metric sinks based on theMetricsSink
configuration, including in-memory,statsd
, anddogstatsd
sinks.180-184: The
gatherPrometheus
function includes a check to ensure Prometheus metrics are enabled before attempting to gather them, which is part of the improved error handling.206-216: The
gatherGeneric
function is updated to use theDisplayableSink
interface for displaying metrics, enabling a more flexible display of metrics.122-125: Please verify if the issues raised in previous comments regarding the
defer
statement within thecase "mem":
block and thenil
check forsink
before adding it to theFanoutSink
have been addressed in the current code.Also applies to: 138-138
@tac0turtle are you good with this? seems like tests are just weird flakiness |
b782d7d
to
9ef410e
Compare
@itsdevbear made the inmemsink the default on the switch case, let me know if youre fine with that. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 3
Configuration used: CodeRabbit UI
Files ignored due to filter (4)
- x/mint/go.mod
- x/nft/go.mod
- x/slashing/go.mod
- x/staking/go.mod
Files selected for processing (3)
- server/config/config.go (1 hunks)
- telemetry/metrics.go (8 hunks)
- telemetry/metrics_test.go (2 hunks)
Additional comments: 2
server/config/config.go (1)
- 234-238: The addition of
MetricsSink
to theTelemetry
configuration inDefaultConfig
is consistent with the PR's objective to enablestatsd
anddogstatsd
telemetry sinks. Ensure thattelemetry.MetricSinkInMem
is the intended default value forMetricsSink
.telemetry/metrics_test.go (1)
- 20-25: The addition of the
MetricsSink
field to theConfig
struct in theTestMetrics_InMem
function is consistent with the PR's objective to enhance telemetry features. This change allows the test to specify the in-memory sink for metrics.
617ee04
to
4905617
Compare
) Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: Aleksandr Bezobchuk <[email protected]> Co-authored-by: marbar3778 <[email protected]> Co-authored-by: Marko <[email protected]> (cherry picked from commit 3ba1c5b) # Conflicts: # CHANGELOG.md # client/v2/go.mod # go.mod # simapp/go.mod # simapp/gomod2nix.toml # tests/go.mod # tests/starship/tests/go.mod # tests/starship/tests/go.sum # tools/confix/data/v0.51-app.toml # x/accounts/go.mod # x/accounts/go.sum # x/auth/go.mod # x/auth/go.sum # x/authz/go.mod # x/authz/go.sum # x/bank/go.mod # x/bank/go.sum # x/circuit/go.mod # x/distribution/go.mod # x/distribution/go.sum # x/evidence/go.mod # x/feegrant/go.mod # x/gov/go.mod # x/gov/go.sum # x/group/go.mod # x/group/go.sum # x/mint/go.mod # x/mint/go.sum # x/nft/go.mod # x/params/go.mod # x/params/go.sum # x/protocolpool/go.mod # x/protocolpool/go.sum # x/slashing/go.mod # x/slashing/go.sum # x/staking/go.mod # x/staking/go.sum # x/upgrade/go.mod
…kport #18646) (#18673) Co-authored-by: Julien Robert <[email protected]>
* feat: secp256k1 public key constant time (cosmos#18026) Signed-off-by: bizk <[email protected]> * chore: Fixed changelog duplicated items (cosmos#18628) * adr: Un-Ordered Transaction Inclusion (cosmos#18553) Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: Aleksandr Bezobchuk <[email protected]> * docs: lint ADR-070 (cosmos#18634) * fix(baseapp)!: postHandler should run regardless of result (cosmos#18627) * docs: fix typos in adr-007-specialization-groups.md (cosmos#18635) * chore: alphabetize labels (cosmos#18640) * docs(x/circuit): add note on ante handler (cosmos#18637) Co-authored-by: Aleksandr Bezobchuk <[email protected]> * fix: telemetry metric label variable (cosmos#18643) * chore: typos fix (cosmos#18642) * refactor(store/v2): updates from integration (cosmos#18633) * build(deps): Bump actions/setup-go from 4 to 5 (cosmos#18647) Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com> * feat(store/v2): snapshot manager (cosmos#18458) * chore(client/v2): fix typos in the README.md (cosmos#18657) * fix(baseapp): protocompat.go gogoproto.Merge does not work with custom types (cosmos#18654) Co-authored-by: unknown unknown <unknown@unknown> * chore: fix several minor typos (cosmos#18660) * chore(tools/confix/cmd): fix typo in view.go (cosmos#18659) * refactor(x/staking): check duplicate addresses in StakeAuthorization's params (cosmos#18655) * feat(accounts): use gogoproto API instead of protov2. (cosmos#18653) Co-authored-by: unknown unknown <unknown@unknown> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * fix(store/commitment/iavl): honor tree.Remove error firstly (cosmos#18651) * build(deps): Bump actions/stale from 8 to 9 (cosmos#18656) Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * chore(docs): fix typos & wording in docs (cosmos#18667) * chore: fix several typos. (cosmos#18666) * feat(telemetry): enable `statsd` and `dogstatsd` telemetry sinks (cosmos#18646) Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: Aleksandr Bezobchuk <[email protected]> Co-authored-by: marbar3778 <[email protected]> Co-authored-by: Marko <[email protected]> * feat(store/v2): add SetInitialVersion in SC (cosmos#18665) * feat(client/keys): support display discreetly for `keys add` (cosmos#18663) Co-authored-by: Julien Robert <[email protected]> * ci: add misspell action (cosmos#18671) * chore: typos fix by misspell-fixer (cosmos#18683) Co-authored-by: github-merge-queue <[email protected]> Co-authored-by: Julien Robert <[email protected]> * chore: add v0.50.2 changelog to main (cosmos#18682) * build(deps): Bump github.com/jhump/protoreflect from 1.15.3 to 1.15.4 in /tests (cosmos#18678) Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * refactor(bank): remove .String() calls (cosmos#18175) Co-authored-by: Facundo <[email protected]> * ci: use codespell instead of misspell-fixer (cosmos#18686) Co-authored-by: Marko <[email protected]> * feat(gov): add proposal types and spam votes (cosmos#18532) * feat(accounts): use account number as state prefix for account state (cosmos#18664) Co-authored-by: unknown unknown <unknown@unknown> * chore: typos fixes by cosmos-sdk bot (cosmos#18689) Co-authored-by: github-merge-queue <[email protected]> Co-authored-by: Julien Robert <[email protected]> Co-authored-by: marbar3778 <[email protected]> * feat(client/keys): support display discreetly for keys mnemonic (cosmos#18688) * refactor: remove panic usage in keeper methods (cosmos#18636) * ci: rename pr name in misspell job (cosmos#18693) Co-authored-by: Marko <[email protected]> * build(deps): Bump github.com/pelletier/go-toml/v2 from 2.1.0 to 2.1.1 in /tools/confix (cosmos#18702) Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com> * feat(client/keys): support display discreetly for keys export (cosmos#18684) * feat(x/gov): better gov genesis validation (cosmos#18707) --------- Signed-off-by: bizk <[email protected]> Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: Carlos Santiago Yanzon <[email protected]> Co-authored-by: yihuang <[email protected]> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: Aleksandr Bezobchuk <[email protected]> Co-authored-by: Facundo Medica <[email protected]> Co-authored-by: Akaonetwo <[email protected]> Co-authored-by: Marko <[email protected]> Co-authored-by: Julien Robert <[email protected]> Co-authored-by: dreamweaverxyz <[email protected]> Co-authored-by: Pioua <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: cool-developer <[email protected]> Co-authored-by: leonarddt05 <[email protected]> Co-authored-by: testinginprod <[email protected]> Co-authored-by: unknown unknown <unknown@unknown> Co-authored-by: Sukey <[email protected]> Co-authored-by: axie <[email protected]> Co-authored-by: Luke Ma <[email protected]> Co-authored-by: Emmanuel T Odeke <[email protected]> Co-authored-by: 0xn4de <[email protected]> Co-authored-by: hattizai <[email protected]> Co-authored-by: Devon Bear <[email protected]> Co-authored-by: Marko <[email protected]> Co-authored-by: Halimao <[email protected]> Co-authored-by: Cosmos SDK <[email protected]> Co-authored-by: github-merge-queue <[email protected]> Co-authored-by: Facundo <[email protected]> Co-authored-by: Likhita Polavarapu <[email protected]>
Description
Closes: #XXXX
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!
in the type prefix if API or client breaking changeCHANGELOG.md
Reviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
I have...
Summary by CodeRabbit
New Features
Enhancements
x/gov
module for direct proposal queries.Configuration Changes
metrics-backend
,statsd-addr
, anddatadog-hostname
settings to telemetry configuration.Bug Fixes
Documentation
Refactor
Tests
MetricsSink
configuration field.Chores
set -euo pipefail
.