Skip to content
This repository was archived by the owner on Feb 18, 2025. It is now read-only.

Replaced stopwatch with a shared-stopwatch suitable for sharing between go routines#273

Merged
shlomi-noach merged 2 commits intoopenark:shared-stopwatchfrom
maurosr:master
Sep 17, 2017
Merged

Replaced stopwatch with a shared-stopwatch suitable for sharing between go routines#273
shlomi-noach merged 2 commits intoopenark:shared-stopwatchfrom
maurosr:master

Conversation

@maurosr
Copy link

@maurosr maurosr commented Aug 22, 2017

This PR fixes the discovery timing metrics.
Also fixes an issue with the json encoding of discovery metrics.

This shared stopwatch can be shared between multiple go routines. It will record elapsed real time, i.e. all time where at least one routine was doing a certain task. For example, if routine1 started work at 07:00:01 and ended at 07:00:06, routine2 started work at 07:00:03 and ended at 07:00:09 then elapsed time will be 8s.

Mauro Schilman and others added 2 commits August 22, 2017 12:00
…en go routines.

Fixed discovery timing metrics.
Fixed json encoding of discovery metrics.
@shlomi-noach
Copy link
Collaborator

Apologies, taking me a while to get to this.

@maurosr
Copy link
Author

maurosr commented Aug 31, 2017

No rush, the stopwatch is not clicking... yet :P

Copy link
Collaborator

@shlomi-noach shlomi-noach left a comment

Choose a reason for hiding this comment

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

I'm just wondering here what is the meaning of summing up the latencies where some of the operations run in parallel?
The sum of latencies is a metric, but does not depict overall run time, which may be smaller than the sum.

@maurosr
Copy link
Author

maurosr commented Aug 31, 2017

So, it is not recording the sum of the latencies, but indeed the overall run time from the app point of view. Another way to see it is: if you have N workers in your house, the stopwatch measures "how long did they take to put the floor", not "how many man-hours did it took to put the floor".

@shlomi-noach
Copy link
Collaborator

Thank you for clarifying!

@maurosr
Copy link
Author

maurosr commented Sep 13, 2017

Friendly ping.

@shlomi-noach
Copy link
Collaborator

Naughty @sjmudd . I asked him to relay that I will be busy finalizing a release of orchestrator (As I did yesterday) and will only then proceed to address Booking.com's PRs, as well as deliver my apologies for the delay.

@shlomi-noach shlomi-noach changed the base branch from master to shared-stopwatch September 17, 2017 06:57
@shlomi-noach
Copy link
Collaborator

I'm going to merge this into a local branch called "shared-stopwatch", and test it in production for a while.

@shlomi-noach
Copy link
Collaborator

By the way, @maurosr I encourage you to not work on your master branch but on a separate branch.

@shlomi-noach
Copy link
Collaborator

@maurosr please see review and continued discussion in #298

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants