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

Full signatures & remove signatures from health_check #931

Merged
merged 4 commits into from Oct 14, 2020

Conversation

raymondjacobson
Copy link
Member

@raymondjacobson raymondjacobson commented Oct 14, 2020

Trello Card Link

https://trello.com/c/inyb5DjL/1609-fix-full-models-to-include-signatures-remove-sigs-from-health-check

Description

  • Removes signatures from /health_check routes
    Signatures are actually pretty expensive.
empty {} response, with signatures:   254 req/s and 900ms median
empty {} response without signatures: 486 req/s, median resp is 10ms.

We do need to keep them for most endpoints, but we don't need them for health check

  • Moves success_response_backwards_compat from discprov (All of our services should have upgraded to libs past this / they absolutely will need to for new contracts)

  • Adds wallet to full response for user object

  • Adds signatures to v1/full

Services

  • Discovery Provider

Does it touch a critical flow like Discovery indexing, Creator Node track upload, Creator Node gateway, or Creator Node file system?

Delete an option.

  • ✅ Nope

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide repro instructions & any configuration.
Include log analysis if applicable.

  1. Manually hit each /full endpoint locally
  2. Verified health check, block check, and play check endpoints

@raymondjacobson raymondjacobson changed the title Rj full sigs Full signatures & remove signatures from health_check Oct 14, 2020
Copy link
Contributor

@dmanjunath dmanjunath left a comment

Choose a reason for hiding this comment

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

Everything looks good except for removing the backwards compat. Unfortunately I think there's some legacy code that still expects the old style dp responses

@@ -30,23 +30,16 @@ def default(self, o): # pylint: disable=E0202
def error_response(error, error_code=500):
return jsonify({'success': False, 'error': error}), error_code

# Create a response dict with just data, signature, and timestamp
# This response will contain a duplicate of response_entity
def success_response_backwards_compat(response_entity=None, status=200):
Copy link
Contributor

Choose a reason for hiding this comment

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

unfortunately i don't think we can remove this quite yet. i think while libs are upgraded, there are places that are still using the old format https://github.com/AudiusProject/audius-protocol/blob/master/identity-service/src/routes/healthCheck.js#L216

Copy link
Member Author

Choose a reason for hiding this comment

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

fair enough. won't remove yet :)

@raymondjacobson raymondjacobson merged commit 5496c36 into master Oct 14, 2020
@raymondjacobson raymondjacobson deleted the rj-full-sigs branch October 14, 2020 03:43
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.

None yet

2 participants