search

change-metrics / monocle

Monocle helps teams and individual to better organize daily duties and to detect anomalies in the way changes are produced and reviewed.
https://changemetrics.io
GNU Affero General Public License v3.0
372 stars 58 forks source link

Duration is missing in some ChangeCommentedEvent and ChangeReviewedEvent #1092

Open leonid-deriv opened 9 months ago

leonid-deriv commented 9 months ago

Probably 50% of the events above do not have the duration field. It is not clear why. Is this a bug or a feature?

morucci commented 9 months ago

Hi, the duration field is optional and according to the code only merged or closed PRs got the duration field. https://github.com/change-metrics/monocle/blob/master/src/Lentille/GitHub/Utils.hs#L183

leonid-deriv commented 9 months ago

is this really correct? does it matter for the events above that PR is closed or merged. Or do you mean that duration will be added to these events when PR is closed?

morucci commented 9 months ago

Yes when the PR is merged or closed then the duration is added to the Change object and related Event objects.

leonid-deriv commented 9 months ago

Maybe I am wrong but it will be less complex to add duration at the time we create events. Plus, as I mentioned above, there are benefits in having this information even if PR is not merged yet - what is the downside ?. I can be wrong of course :)

morucci commented 9 months ago

Yes we could set the duration for not merged/closed Changes however the duration won't be accurate most of the time if the Change is not merged or closed. That sounds more logical to let the data consumer (eg. the Monocle UI) figures out the (change in-progress) duration at query time. Could the scripted fields help for your use case https://www.elastic.co/guide/en/elasticsearch/reference/current/search-fields.html#script-fields ? What is the use case ?

leonid-deriv commented 9 months ago

The metric is PR pickup time - when there is a first comment. Probably I can filter by only merge/closed PRs but I feel this it does not matter what will happen with PR. Also I think the time should be consistent because I understand we measure the time between PR creation and comment and PR creation time will be the same irrespective of whether you merged it or not. No?

morucci commented 9 months ago

The duration on the Event object is the same as the duration on a Change object.

Perhaps what you would like is a two new fields:

These new fields could be added to Change objects ?

leonid-deriv commented 9 months ago

For me one field will be enough - it does not matter whether it was a comment or review (not sure why GitHub has two different events)