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.

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

chore: more granular metrics for routes/quotes fetched #756

Closed
wants to merge 1 commit into from

Conversation

xrsv
Copy link
Contributor

@xrsv xrsv commented Oct 30, 2024

  • What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
    Adds more granular metrics for routes/quotes fetched.
    This will give us more visibility on onChain effort, total routes, total quotes fetched per chain.

  • What is the current behavior? (You can also link to an open issue here)
    Only logging number of quotes/latency at the top level

  • What is the new behavior (if this is a feature change)?
    Logging quotes/routes/latency at top + per chain level.

  • Other information:

@xrsv xrsv requested a review from a team as a code owner October 30, 2024 17:45
@xrsv xrsv requested review from jsy1218 and cgkol October 30, 2024 17:45
Copy link

graphite-app bot commented Oct 30, 2024

Graphite Automations

"Request reviewers once CI passes on smart-order-router repo" took an action on this PR • (10/30/24)

1 reviewer was added and 1 assignee was added to this PR based on 's automation.


metric.putMetric(
`MixedRoutesFetched_Chain_${this.chainId}`,
_(routesWithQuotes).sum()
Copy link
Member

Choose a reason for hiding this comment

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

number of routes with quotes should just be routesWithQuotes.length?

.sum()
);

metric.putMetric(`V2RoutesFetched`, _(routesWithQuotes).sum());
Copy link
Member

Choose a reason for hiding this comment

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

so this logs the routes fetched in case we have a cache routes miss. in case we have a cached routes hit, it will be within refreshRoutesThenGetQuotes.

I'm thinking (1) whether we should log routes and/or quotes fetched in case of cached routes hit (2) whether we should log both cached routes hit and cached routes miss

Copy link
Contributor Author

Choose a reason for hiding this comment

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

makes sense - lets chat

@jsy1218
Copy link
Member

jsy1218 commented Oct 30, 2024

good stuff, let's sync on the logging details

@xrsv xrsv closed this Oct 30, 2024
@xrsv
Copy link
Contributor Author

xrsv commented Oct 30, 2024

discussed offline - will add logging in a different place

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