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

feat: Add onBreak handler to token price service #3677

Merged
merged 1 commit into from
Dec 19, 2023

Conversation

Gudahtt
Copy link
Member

@Gudahtt Gudahtt commented Dec 18, 2023

Explanation

The CodefiTokenPricesServiceV2 service now accepts an onBreak event handler. This handler is called upon each circuit break. This allows us to collect metrics on circuit breaks, which would help is detect widespread failures of this service.

References

N/A

Changelog

@metamask/assets-controllers

  • Added: Add onBreak option to CodefiTokenPricesServiceV2

Checklist

  • I've updated the test suite for new or updated code as appropriate
  • I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate
  • I've highlighted breaking changes using the "BREAKING" category above as appropriate

@Gudahtt Gudahtt force-pushed the add-break-handler-to-token-prices-service branch from 955784e to fac7564 Compare December 18, 2023 14:01
@Gudahtt Gudahtt marked this pull request as ready for review December 18, 2023 14:27
@Gudahtt Gudahtt requested a review from a team as a code owner December 18, 2023 14:27
Copy link
Contributor

@mcmire mcmire left a comment

Choose a reason for hiding this comment

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

Looks good. I'm wondering if we should consider pulling this out so that all token price services get this behavior. Sahar is adding a function that can be called outside of the context of the controller here, so that might be a good place. But we can think about that later if it makes sense.

@Gudahtt
Copy link
Member Author

Gudahtt commented Dec 19, 2023

We'd have to add a constructor to the abstract price service class to make that possible, and I'm not sure that would be worth the effort, especially since we only have one service in practice. The simple object type we use for it now is convenient for testing.

Edit: Oh, maybe you meant adding a "subscribeToBreak" method, something like that? Can't think of a reason why that would be useful. This was meant for tracking outages, which we only need to do in one place.

The `CodefiTokenPricesServiceV2` service now accepts an `onBreak` event
handler. This handler is called upon each circuit break. This allows us
to collect metrics on circuit breaks, which would help is detect
widespread failures of this service.
@Gudahtt Gudahtt force-pushed the add-break-handler-to-token-prices-service branch from fac7564 to a514d1b Compare December 19, 2023 14:39
@Gudahtt Gudahtt merged commit c437148 into main Dec 19, 2023
136 checks passed
@Gudahtt Gudahtt deleted the add-break-handler-to-token-prices-service branch December 19, 2023 14:45
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