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

Drop liquidity orders from matched orders metric #167

Merged
merged 2 commits into from
Apr 27, 2022

Conversation

MartinquaXD
Copy link
Contributor

Fixes #129

The "Matched Orders" panel in Grafana currently includes liquidity orders. If there are many liquidity orders the graph for matched orders declines significantly making it seem like our service is experiencing issues.
This can be solved by introducing a metric which only tracks how many user orders have been created. If we use this metric in the Grafana panel the "Matched Orders" graph will better reflect the user experience of the service.

Test Plan

Local test submitting 1 user order and 1 liquidity order.
New metric counting user orders:

# HELP gp_v2_api_orderbook_user_orders_created Number of user (non-liquidity) orders created.
# TYPE gp_v2_api_orderbook_user_orders_created counter
gp_v2_api_orderbook_user_orders_created 1

Metric we previously in the Grafana panel

# HELP gp_v2_api_api_requests_complete Number of completed API requests.
# TYPE gp_v2_api_api_requests_complete counter
gp_v2_api_api_requests_complete{method="v1/create_order",status_code="201"} 2

This shows that the new metrics is working and ignores liquidity orders.

@MartinquaXD MartinquaXD requested a review from a team as a code owner April 25, 2022 07:32
Copy link
Contributor

@nlordell nlordell left a comment

Choose a reason for hiding this comment

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

LGTM.

As a side note I think that we should add the is_liquidity_order flag to the OrderMetadata as well - this is so we don't need to ensure that the API and service are configured with the same liquidity owners.

@codecov-commenter
Copy link

codecov-commenter commented Apr 27, 2022

Codecov Report

Merging #167 (9913fe3) into main (1416209) will decrease coverage by 0.00%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main     #167      +/-   ##
==========================================
- Coverage   64.83%   64.82%   -0.01%     
==========================================
  Files         185      185              
  Lines       38388    38398      +10     
==========================================
+ Hits        24889    24893       +4     
- Misses      13499    13505       +6     

@MartinquaXD MartinquaXD merged commit f203851 into main Apr 27, 2022
@MartinquaXD MartinquaXD deleted the fix-matched-orders-metrics branch April 27, 2022 06:35
@nlordell
Copy link
Contributor

@MartinquaXD Does this also fix our time to trade metric?

@MartinquaXD
Copy link
Contributor Author

MartinquaXD commented Apr 27, 2022

Does this also fix our time to trade metric?

No, it doesn't. But looking at the code the fix should be trivial. I'll submit a PR.

nargeslein pushed a commit to nargeslein/services that referenced this pull request Apr 29, 2022
nargeslein pushed a commit to nargeslein/services that referenced this pull request Apr 30, 2022
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.

Better Tracking Of Order Trading Metrics
4 participants