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.

Sign up for GitHub

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

Jump to bottom

feat: emit more data in prom metrics #15657

Merged
merged 10 commits into from
Apr 6, 2023
Merged

feat: emit more data in prom metrics #15657

merged 10 commits into from
Apr 6, 2023

Conversation

dhanusaputra
Copy link
Contributor

@dhanusaputra dhanusaputra commented Apr 1, 2023 •
edited
Loading

Description

Closes: #15593

Workscope

  • add go version to prometheus metrics
  • add software version to metrics
  • add height of next upgrade
  • see if anything else can be added

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...

  • included the correct type prefix in the PR title
  • added ! to the type prefix if API or client breaking change
  • targeted the correct branch (see PR Targeting)
  • provided a link to the relevant issue or specification
  • followed the guidelines for building modules
  • included the necessary unit and integration tests
  • added a changelog entry to CHANGELOG.md
  • included comments for documenting Go code
  • updated the relevant documentation or specification
  • reviewed "Files changed" and left comments if necessary
  • confirmed all CI checks have passed

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...

  • confirmed the correct type prefix in the PR title
  • confirmed ! in the type prefix if API or client breaking change
  • confirmed all author checklist items have been addressed
  • reviewed state machine logic
  • reviewed API design and naming
  • reviewed documentation is accurate
  • reviewed tests and test coverage
  • manually tested (if applicable)

@dhanusaputra dhanusaputra requested a review from a team as a code owner April 1, 2023 06:49
julienrbrt
julienrbrt approved these changes Apr 1, 2023
View reviewed changes
Copy link
Member

@julienrbrt julienrbrt left a comment •
edited
Loading

Choose a reason for hiding this comment

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

lgtm, thank you!
(Edit: I'll try it out with simd in a bit)

telemetry/metrics.go Outdated
}

type plan struct {
Name string `json:"name,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

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

If we only use the height, we do not need to parse the rest.

github-advanced-security[bot]
github-advanced-security bot found potential problems Apr 1, 2023
View reviewed changes
telemetry/metrics.go Fixed Show fixed Hide fixed
@julienrbrt
Copy link
Member

before

image

after

metrics

Do we want this information as global label actually @tac0turtle?

@tac0turtle
Copy link
Member

tac0turtle commented Apr 3, 2023 •
edited
Loading

hmm we shouldnt have it as a global but instead have a single metric

tac0turtle
tac0turtle reviewed Apr 3, 2023
View reviewed changes
server/start.go Outdated
Comment on lines 528 to 529
uk := upgradekeeper.NewKeeper(nil, nil, nil, homePath, nil, "")
if upgradeInfo, err := uk.ReadUpgradeInfoFromDisk(); err == nil {
ls = append(ls, telemetry.NewLabel("update_height", strconv.FormatInt(upgradeInfo.Height, 10)))
}
Copy link
Member

@tac0turtle tac0turtle Apr 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

this can go in the upgrade module. helps avoid the upgrade dep in server

julienrbrt
julienrbrt requested changes Apr 4, 2023
View reviewed changes
Copy link
Member

@julienrbrt julienrbrt left a comment

Choose a reason for hiding this comment

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

image
note: I have tried to check the upgrade height, have you @dhanusaputra?

server/start.go Outdated Show resolved Hide resolved
x/upgrade/module.go Outdated
@@ -223,6 +226,10 @@ func ProvideModule(in ModuleInputs) ModuleOutputs {
m := NewAppModule(k, in.AddressCodec)
gh := govv1beta1.HandlerRoute{RouteKey: types.RouterKey, Handler: NewSoftwareUpgradeProposalHandler(k)}

if upgradePlan, err := k.ReadUpgradeInfoFromDisk(); err == nil {
Copy link
Member

Choose a reason for hiding this comment

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

I have not tried if it works, but if it does it will only work for app wiring app (app_v2). Could you add this additionally to the app.go?

x/upgrade/module.go Outdated
@@ -223,6 +226,10 @@ func ProvideModule(in ModuleInputs) ModuleOutputs {
m := NewAppModule(k, in.AddressCodec)
gh := govv1beta1.HandlerRoute{RouteKey: types.RouterKey, Handler: NewSoftwareUpgradeProposalHandler(k)}

if upgradePlan, err := k.ReadUpgradeInfoFromDisk(); err == nil {
telemetry.SetGaugeWithLabels([]string{"server", "info"}, 1, []metrics.Label{telemetry.NewLabel("upgrade_height", strconv.FormatInt(upgradePlan.Height, 10))})
Copy link
Member

@julienrbrt julienrbrt Apr 4, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Suggested change
telemetry.SetGaugeWithLabels([]string{"server", "info"}, 1, []metrics.Label{telemetry.NewLabel("upgrade_height", strconv.FormatInt(upgradePlan.Height, 10))})
if upgradePlan.Height > 0 {
telemetry.SetGaugeWithLabels([]string{"server", "info"}, 1, []metrics.Label{telemetry.NewLabel("upgrade_height", strconv.FormatInt(upgradePlan.Height, 10))})
}

Prob need this because the app is instantiated the first time with a temporary directory, for getting the application encoding (

cosmos-sdk/simapp/simd/cmd/root.go

Line 47 in c1ea84d

tempApp := simapp.NewSimApp(log.NewNopLogger(), dbm.NewMemDB(), nil, true, simtestutil.NewAppOptionsWithFlagHome(tempDir()))
)

@dhanusaputra
Copy link
Contributor Author

note: I have tried to check the upgrade height, have you @dhanusaputra?

not yet, will try run the simapp with better PC soon

tac0turtle
tac0turtle reviewed Apr 4, 2023
View reviewed changes
x/upgrade/module.go Outdated
@@ -223,6 +226,10 @@ func ProvideModule(in ModuleInputs) ModuleOutputs {
m := NewAppModule(k, in.AddressCodec)
gh := govv1beta1.HandlerRoute{RouteKey: types.RouterKey, Handler: NewSoftwareUpgradeProposalHandler(k)}

if upgradePlan, err := k.ReadUpgradeInfoFromDisk(); err == nil && upgradePlan.Height > 0 {
Copy link
Member

Choose a reason for hiding this comment

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

this will only set it on startup, we should set it where its called the plan is set. also this assumes the application is using depinject which isnt the case yet.

cosmos-sdk/x/upgrade/keeper/keeper.go

Line 194 in 92738c0

func (k Keeper) ScheduleUpgrade(ctx sdk.Context, plan types.Plan) error {

@dhanusaputra
Copy link
Contributor Author

no luck testing the upgrade height
tried this guide but the cosmovisor keep running without dumping new version, not sure why
image

tac0turtle
tac0turtle reviewed Apr 6, 2023
View reviewed changes
simapp/app.go Outdated Show resolved Hide resolved
tac0turtle
tac0turtle approved these changes Apr 6, 2023
View reviewed changes
Copy link
Member

@tac0turtle tac0turtle left a comment

Choose a reason for hiding this comment

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

utACK

@julienrbrt julienrbrt added the A:automerge Automatically merge PR once all prerequisites pass. label Apr 6, 2023
@julienrbrt julienrbrt enabled auto-merge (squash) April 6, 2023 12:41
@julienrbrt julienrbrt merged commit 17debf2 into cosmos:main Apr 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tac0turtle tac0turtle tac0turtle approved these changes

@julienrbrt julienrbrt julienrbrt approved these changes

@samricotta samricotta Awaiting requested review from samricotta

Assignees

@tac0turtle tac0turtle

@julienrbrt julienrbrt

Labels
A:automerge Automatically merge PR once all prerequisites pass. C:x/upgrade
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

emit more data in prom metrics
3 participants
@dhanusaputra @julienrbrt @tac0turtle