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.

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

Deprecate :{start,end}_time, add :timing instead #526

Merged
merged 1 commit into from
Dec 20, 2019
Merged

Conversation

kyrylo
Copy link
Contributor

@kyrylo kyrylo commented Dec 19, 2019

There are a few reasons for this change:

  1. We use realtime to measure performance for the :total stat. This is no
    good, especially since we use monotonic time for other groups. It is
    inconsitent and confusing

  2. New functionality requires a better API. Specifically, we want to be able to
    send timing of a certain job. To achieve that we wrap the job in a benchmark
    measure call. The problem is that our API doesn't make it easy to send
    already calculated timing. start_time makes the user code very clunky and
    confusing

Before:

start_time = Time.now
calculate_operation

Airbrake.notify_request(
  ...,
  start_time: start_time,
  end_time: Time.now,
)

After:

timing = Airbrake::Benchmark.measure do
  calculate_operation
end

Airbrake.notify_request(
  ...,
  timing: timing,
)

spec/performance_notifier_spec.rb Outdated Show resolved Hide resolved
spec/performance_notifier_spec.rb Outdated Show resolved Hide resolved
There are a few reasons for this change:

1. We use realtime to measure performance for the `:total` stat. This is no
   good, especially since we use monotonic time for other groups. It is
   inconsitent and confusing

2. New functionality requires a better API. Specifically, we want to be able to
   send timing of a certain job. To achieve that we wrap the job in a benchmark
   `measure` call. The problem is that our API doesn't make it easy to send
   already calculated timing. `start_time` makes the user code very clunky and
   confusing

Before:

```
start_time = Time.now
calculate_operation

Airbrake.notify_request(
  ...,
  start_time: start_time,
  end_time: Time.now,
)
```

After:

```
timing = Airbrake::Benchmark.measure do
  calculate_operation
end

Airbrake.notify_request(
  ...,
  timing: timing,
)
```
@kyrylo kyrylo merged commit 066c602 into master Dec 20, 2019
@kyrylo kyrylo deleted the timing-param branch December 20, 2019 08:25
kyrylo added a commit to airbrake/airbrake that referenced this pull request Jan 8, 2020
Resolves airbrake/airbrake-ruby#532
(Airbrake performance changes are filling logs with deprecation messages)

This was inroduced by airbrake/airbrake-ruby#526
We can now pass `event.duration`, so there's no need to calculate it again. This
is more accurate as well since we pass monotonic time instead of realtime.
kyrylo added a commit to airbrake/airbrake that referenced this pull request Jan 8, 2020
Resolves airbrake/airbrake-ruby#532
(Airbrake performance changes are filling logs with deprecation messages)

This was inroduced by airbrake/airbrake-ruby#526
We can now pass `event.duration`, so there's no need to calculate it again. This
is more accurate as well since we pass monotonic time instead of realtime.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants