-
Notifications
You must be signed in to change notification settings - Fork 415
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
add support for e2e latency for dynamic mode and adoption rate metrics #690
add support for e2e latency for dynamic mode and adoption rate metrics #690
Conversation
Welcome @minj131! |
Hi @minj131. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
6575eda
to
1ef4cd0
Compare
/ok-to-test |
1ef4cd0
to
0ab9b1e
Compare
8be18a5
to
4a8f577
Compare
9c6ccfb
to
6768da7
Compare
/re-test |
/retest |
/lgtm |
/ok-to-test |
e7bc88c
to
771673c
Compare
771673c
to
f62a733
Compare
f62a733
to
af38dc7
Compare
pkg/fileutil/util.go
Outdated
@@ -104,3 +106,19 @@ func StartLoadDynamicFile(filename string, callBack FileChangeCallBack, stopCh < | |||
} | |||
}, time.Second, stopCh) | |||
} | |||
|
|||
func CalculateTimeDeltaFromUnixInSeconds(from string, to int64) (float64, error) { |
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.
We are converting the data type from int64 to string and converting it back to int64 for comparison.
Please make it more focused by passing (string, int64) and only converting the first argument to int64.
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.
change the param names to startTime and endTime
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.
can we drop the to parameter completely? The callers are passing time.Now anyway.
} | ||
|
||
type DynamicFileData struct { | ||
// Time that the object takes from update time to load time | ||
LastUpdatedDateTime string `json:"LastUpdatedDateTime"` |
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.
This being a string is going to cause problems in the future. We have to migrate this to the right representation(epoch time is always int64) in a future change.
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.
Will take an AI, this requires changes across all components unfortunately
09fe619
to
784221d
Compare
42e7769
to
3099468
Compare
3099468
to
1088ba2
Compare
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
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jyotimahapatra, minj131 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/lgtm |
What this PR does / why we need it:
This adds logic to support e2e latency for dynamic mode and dynamic backend mode files.
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close the issue(s) when PR gets merged):Fixes #