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

[Heartbeat] Report next_run info per event #13672

Closed
wants to merge 5 commits into from

Conversation

andrewvc
Copy link
Contributor

Add support for next_run and next_run_in fields to the monitors object in events. This allows for the computation of SLA statistics in elasticsearch.

Add support for `next_run` and `next_run_in` fields to the monitors
object in events. This allows for the computation of SLA statistics in
elasticsearch.
@andrewvc andrewvc added enhancement Heartbeat Team:obs-ds-hosted-services Label for the Observability Hosted Services team labels Sep 13, 2019
@andrewvc andrewvc requested a review from ruflin September 13, 2019 14:30
@andrewvc andrewvc requested a review from a team as a code owner September 13, 2019 14:30
@andrewvc andrewvc self-assigned this Sep 13, 2019
@elasticmachine
Copy link
Collaborator

Pinging @elastic/uptime

@@ -63,6 +63,21 @@
description: >
A token unique to a simultaneously invoked group of checks as in the case where multiple IPs are checked for a single DNS entry.

- name: next_run
Copy link
Contributor

@ruflin ruflin Sep 16, 2019

Choose a reason for hiding this comment

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

I normally stumble over names which are foo and foo_bar as it seems foo should probably be an object because the two are somehow related. I guess this is the case here, but not sure what I should recommend as a name ...

BTW: 100% personal preference.

I think I also missed the reasoning why we need both. Wouldn't one be enough?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good point. I think we only need next_run_in. This is in support of timelines in the future. Let me leave this PR open for now, when we get the Kibana side of this going and we're sure this does everything we need I can come back to this and merge it.

Copy link
Contributor

Choose a reason for hiding this comment

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

SGTM. Ping me when I should have a look again.

Copy link
Contributor

@ruflin ruflin left a comment

Choose a reason for hiding this comment

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

Code wise LGTM. Left one question.

Could you make hound happy?

@@ -20,6 +20,8 @@ package monitors
import (
"github.com/pkg/errors"

"github.com/elastic/beats/heartbeat/scheduler/schedule"

Copy link
Contributor

Choose a reason for hiding this comment

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

usual nit

@@ -55,7 +61,7 @@ func Parse(in string) (*Schedule, error) {
return &Schedule{s}, nil
}

func (s intervalScheduler) Next(t time.Time) time.Time {
func (s IntervalScheduler) Next(t time.Time) time.Time {

Choose a reason for hiding this comment

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

exported method IntervalScheduler.Next should have comment or be unexported

interval time.Duration
}

func (s IntervalScheduler) QuantizedPeriod(now time.Time) (gte time.Time, lt time.Time) {

Choose a reason for hiding this comment

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

exported method IntervalScheduler.QuantizedPeriod should have comment or be unexported

@@ -29,10 +31,14 @@ type Schedule struct {
scheduler.Schedule
}

type intervalScheduler struct {
type IntervalScheduler struct {

Choose a reason for hiding this comment

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

exported type IntervalScheduler should have comment or be unexported


import "time"

func Quantize(t time.Time, period time.Duration) (start time.Time, end time.Time) {

Choose a reason for hiding this comment

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

exported function Quantize should have comment or be unexported

andrewvc added a commit that referenced this pull request Dec 16, 2019
This field is needed to optimize the overview page queries. By adding the time range this monitor is responsible for we can use a range query to substantially narrow the candidate document set in queries.

This takes over from the experiments in #13672 and should be production quality. It only adds this single necessary field.
andrewvc added a commit to andrewvc/beats that referenced this pull request Dec 16, 2019
This field is needed to optimize the overview page queries. By adding the time range this monitor is responsible for we can use a range query to substantially narrow the candidate document set in queries.

This takes over from the experiments in elastic#13672 and should be production quality. It only adds this single necessary field.

(cherry picked from commit 416aab0)
andrewvc added a commit that referenced this pull request Dec 18, 2019
This field is needed to optimize the overview page queries. By adding the time range this monitor is responsible for we can use a range query to substantially narrow the candidate document set in queries.

This takes over from the experiments in #13672 and should be production quality. It only adds this single necessary field.

(cherry picked from commit 416aab0)
@mergify
Copy link
Contributor

mergify bot commented Apr 7, 2021

This pull request is now in conflicts. Could you fix it? 🙏
To fixup this pull request, you can check out it locally. See documentation: https://help.github.com/articles/checking-out-pull-requests-locally/

git fetch upstream
git checkout -b periods upstream/periods
git merge upstream/master
git push upstream periods

@mergify
Copy link
Contributor

mergify bot commented Sep 22, 2021

This pull request does not have a backport label. Could you fix it @andrewvc? 🙏
To fixup this pull request, you need to add the backport labels for the needed
branches, such as:

  • backport-v./d./d./d is the label to automatically backport to the 7./d branch. /d is the digit

NOTE: backport-skip has been added to this pull request.

@mergify mergify bot added the backport-skip Skip notification from the automated backport with mergify label Sep 22, 2021
@jlind23
Copy link
Collaborator

jlind23 commented Mar 31, 2022

@andrewvc - Closing this one as there were no activity for a while

@jlind23 jlind23 closed this Mar 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-skip Skip notification from the automated backport with mergify enhancement Heartbeat Team:obs-ds-hosted-services Label for the Observability Hosted Services team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants