-
Notifications
You must be signed in to change notification settings - Fork 0
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
support non-integer request ids #83
Conversation
5d8e6e8
to
d6955c1
Compare
// search for any request metrics during the time window for particular request methods | ||
func findMetricsInWindowForMethods(db database.PostgresClient, startTime time.Time, endTime time.Time, testedmethods []string) []database.ProxiedRequestMetric { | ||
// search for any request metrics between starTime and time.Now() for particular request methods | ||
func findMetricsInWindowForMethods(db database.PostgresClient, startTime time.Time, testedmethods []string) []database.ProxiedRequestMetric { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
every occurrence of this helper func used time.Now()
as the endTime
so i made that an assumption of the func.
this allows us to secretly sleep for 10 microseconds in order to allow metrics to actually be created.
doing so fixes frequent but random test failures that look like expected 4 metrics but found 2
or [] does not contain yada yada
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
fyi: recently we added additional CI job, which triggers front-end e2e tests: https://github.com/Kava-Labs/kava-proxy-service/actions/runs/6907131300/job/18800458169
the only issue to make sure it works properly, I'd recommend such steps:
- merge to master
- wait until kava-proxy-service will be deployed to internal-testnet
- retrigger this job: https://github.com/Kava-Labs/kava-proxy-service/blob/main/.github/workflows/ci-webapp-e2e-tests.yml
without waiting and re-triggering it may be the case, that job is successful, but was run against old internal-testnet (front-end e2e tests are run against internal-testnet)
front-end e2e tests helped me to find few very weird bugs (which was impossible to find during unit/e2e tests, for ex. related to CORS, etc...)
also I agree regarding logs, logs are very noisy, definitely need to improve it
EVM requests can have strings as their
id
, not just integers.A brief glimpse at the logs on prod shows that requests with ids that are string uuids or string numbers are failing to decode which causes them to skip the cache and never create metrics.
This PR changes the
ID
field ofEVMRPCRequestEnvelope
fromint64
tointerface{}
in order to support string ids (and even booleans, which return valid responses when queried over the EVM). E2E test confirming requests with stringid
s can now be fielded by the cache is included 👍Other improvements:
require
notassert
so there is clear indication which assertion a failure is coming from