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.

Sign up for GitHub

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

Jump to bottom

fix: Don't send durations when they are not from actual capture times #258

Merged
merged 9 commits into from Oct 3, 2023
Merged

fix: Don't send durations when they are not from actual capture times #258

merged 9 commits into from Oct 3, 2023

Conversation

robbkidd
Copy link
Member

@robbkidd robbkidd commented Sep 29, 2023 •
edited by JamieDanielson

Which problem is this PR solving?

Short description of the changes

  • refactor duration computation into an extracted function
  • test drive timestamp(s) set/unset scenarios

How to verify that this has the expected result

Duration is only set if both timestamps are present. If one or both are missing, attributes are added to the event indicating that, and no duration is set (event timestamp is set to time.now). If both timestamps are present, calculate the duration and add to the event. We should see no more negative durations caused by missing timestamps

robbkidd and others added 2 commits September 29, 2023 15:53
@robbkidd @JamieDanielson
add computeDuration()
0527f6e 
This function does NOT set a duration when we're missing timestamp(s) to
avoid skewing data.

Co-authored-by: Jamie Danielson <JamieDanielson@users.noreply.github.com>
@robbkidd
wip: maybe too much
b99afe7
@robbkidd robbkidd force-pushed the jabb.default-times-when-pair-is-missing branch from 1ec1061 to b99afe7 Compare September 29, 2023 22:52
@JamieDanielson JamieDanielson self-assigned this Oct 2, 2023
@JamieDanielson JamieDanielson added the type: bug Something isn't working label Oct 2, 2023
@robbkidd robbkidd marked this pull request as ready for review October 2, 2023 22:47
@robbkidd robbkidd requested a review from a team as a code owner October 2, 2023 22:47
MikeGoldsmith
MikeGoldsmith approved these changes Oct 3, 2023
View reviewed changes
Copy link
Contributor

@MikeGoldsmith MikeGoldsmith left a comment

Choose a reason for hiding this comment

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

Generally looks good. Added a couple of minor suggestions around styling.

handlers/libhoney_event_handler.go Outdated Show resolved Hide resolved
handlers/libhoney_event_handler.go Show resolved Hide resolved
handlers/libhoney_event_handler.go Outdated Show resolved Hide resolved
@MikeGoldsmith MikeGoldsmith changed the title fix: don't send durations when they are not from actual capture times fix: Don't send durations when they are not from actual capture times Oct 3, 2023
JamieDanielson
JamieDanielson approved these changes Oct 3, 2023
View reviewed changes
Copy link
Contributor

@JamieDanielson JamieDanielson left a comment

Choose a reason for hiding this comment

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

those terms are satisfactory

@JamieDanielson JamieDanielson merged commit ea0b9ad into main Oct 3, 2023
4 checks passed
@JamieDanielson JamieDanielson deleted the jabb.default-times-when-pair-is-missing branch October 3, 2023 20:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MikeGoldsmith MikeGoldsmith MikeGoldsmith approved these changes

@JamieDanielson JamieDanielson JamieDanielson approved these changes

Assignees

@JamieDanielson JamieDanielson

Labels
type: bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

sometimes event duration is < 0
3 participants
@robbkidd @MikeGoldsmith @JamieDanielson