Skip to content

feat(metrics): add price update delay metric and update method #2619

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

Merged
merged 5 commits into from
Apr 29, 2025

Conversation

cctdaniel
Copy link
Contributor

@cctdaniel cctdaniel commented Apr 25, 2025

Summary

Added a new gauge metric pyth_price_update_delay to track the delay between source and target timestamps relative to each price feed's configured threshold. The metric is calculated as:

sourceLatestPricePublishTime - targetLatestPricePublishTime - priceConfigTimeDifference

Rationale

  • Each price feed has its own timeDifference configuration, making static thresholds suboptimal
  • Enables dynamic alerting based on per-feed configuration rather than hardcoded values
  • Handles market hours gracefully - during off-hours, sourceLatestPrice won't update, keeping the delay constant
  • Allows setting unified alerts (e.g. delay > 2min) while respecting individual feed configurations
  • Simplifies monitoring by normalizing delays against their expected thresholds

How has this been tested?

  • Current tests cover my changes
  • Added new tests
  • Manually tested the code

Steps to verify:

  1. Deployed to testnet and confirmed metric appears in Prometheus
  2. Verified calculation is correct by comparing:
    • sourceLatestPrice.publishTime - targetLatestPrice.publishTime against raw timestamps
    • Final gauge value accounts for priceConfigTimeDifference
  3. Checked behavior during market hours and off-hours
  4. Confirmed alerts can be set using simple thresholds that work across all feeds

The changes are non-invasive and purely additive to the metrics system, with no impact on core price pushing functionality.

Copy link

vercel bot commented Apr 25, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
entropy-debugger ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 28, 2025 9:59am
insights ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 28, 2025 9:59am
staking ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 28, 2025 9:59am
3 Skipped Deployments
Name Status Preview Comments Updated (UTC)
api-reference ⬜️ Ignored (Inspect) Visit Preview Apr 28, 2025 9:59am
component-library ⬜️ Ignored (Inspect) Visit Preview Apr 28, 2025 9:59am
proposals ⬜️ Ignored (Inspect) Visit Preview Apr 28, 2025 9:59am

Copy link
Collaborator

@ali-bahjati ali-bahjati left a comment

Choose a reason for hiding this comment

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

Sorry for a lot of change of minds here, i was wondering whether we can expose the raw data behind delay here and calculate the delay in our metrics platform instead. In this way we can expose the details of the delay in the dashboards as well.

@@ -14,6 +14,7 @@ export class PricePusherMetrics {
public lastPublishedTime: Gauge<string>;
public priceUpdateAttempts: Counter<string>;
public priceFeedsTotal: Gauge<string>;
public priceUpdateDelay: Gauge<string>;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Out of curiosity, why are these string :?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

these are for the label names

registers: [this.registry],
});

this.targetTimestamp = new Gauge({
Copy link
Collaborator

Choose a reason for hiding this comment

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

what's the difference between this and lastPublishedTime ?

Copy link
Contributor Author

@cctdaniel cctdaniel Apr 28, 2025

Choose a reason for hiding this comment

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

yeah its redundant, i will remove targetTimestamp and use lastPublishedTime instead

@ali-bahjati
Copy link
Collaborator

Ah please bump the version too.

@cctdaniel cctdaniel requested a review from a team as a code owner April 28, 2025 09:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants