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

[query] Implemented the Graphite integralByInterval function #2596

Merged
merged 32 commits into from
Sep 21, 2020
Merged

[query] Implemented the Graphite integralByInterval function #2596

merged 32 commits into from
Sep 21, 2020

Conversation

teddywahle
Copy link
Contributor

@teddywahle teddywahle commented Sep 4, 2020 •
edited
Loading

What this PR does / why we need it:

Go implementation for the integralByInterval function in Graphite.
Documentation --> https://graphite.readthedocs.io/en/latest/functions.html#graphite.render.functions.integralByInterval

Does this PR introduce a user-facing and/or backwards incompatible change?:

NONE

Does this PR require updating code package or user-facing documentation?:

NONE

@teddywahle teddywahle changed the title [query] Graphite integralByInterval [query] Implemented the Graphite integralByInterval function Sep 4, 2020
robskillington
robskillington reviewed Sep 9, 2020
View reviewed changes
src/query/graphite/native/builtin_functions.go Outdated
var stepCounter int64 = 0

outvals := ts.NewValues(ctx, series.MillisPerStep(), series.Len())
var currentSum float64
Copy link
Collaborator

Choose a reason for hiding this comment

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

It is more Go-ish to combine these var assignments altogether into a multi line var statement (and leave the zero values unassigned, i.e. stepCounter and currentSum):

var (
  stepsPerInterval = intervalUnit.Milliseconds() /  int64(series.MillisPerStep())
  vals = ts.NewValues(ctx, series.MillisPerStep(), series.Len())
  stepCounter int64
  currentSum float64
)

Also "outvals" should either be camel cased (since it's two words) or just one word (i.e. vals).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done! thanks for this.

arnikola
arnikola reviewed Sep 11, 2020
View reviewed changes
src/query/graphite/native/builtin_functions.go
currentSum += n
outvals.SetValueAt(i, currentSum)
}
stepCounter += 1
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it correct to increment step counter if the value is NaN?

Copy link
Contributor Author

@teddywahle teddywahle Sep 21, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Discussed offline. Answer is yes according to graphite-web source code.

src/query/graphite/native/builtin_functions.go
@@ -1000,6 +1000,43 @@ func integral(ctx *common.Context, input singlePathSpec) (ts.SeriesList, error)
return r, nil
}

// integralByInterval will do the same as integral funcion, except it resets the total to 0
// at the given time in the parameter “from”. Useful for finding totals per hour/day/week.
func integralByInterval(ctx *common.Context, input singlePathSpec, intervalString string) (ts.SeriesList, error) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this need to be added to builtin_functions.go?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added

arnikola
arnikola reviewed Sep 21, 2020
View reviewed changes
src/query/graphite/native/builtin_functions.go Outdated
Comment on lines 1102 to 1103
stepCounter int64 = 0
currentSum float64 = 0.0
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: these default to 0 and 0.0 so no need to explicitly use this

arnikola
arnikola approved these changes Sep 21, 2020
View reviewed changes
Copy link
Collaborator

@arnikola arnikola left a comment

Choose a reason for hiding this comment

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

Looks good, thanks for contributing!

@robskillington robskillington merged commit f2ebf5c into m3db:master Sep 21, 2020
ChrisChinchilla pushed a commit that referenced this pull request Oct 5, 2020
Squashed commit of the following:
00c8613 
commit 4f3778dded83ad4d81aaeaad62608f6c7a0f9461
Author: ChrisChinchilla <chris@chronosphere.io>
Date:   Mon Oct 5 12:53:40 2020 +0200

    fix code inclusion

    Signed-off-by: ChrisChinchilla <chris@chronosphere.io>

commit daefb435243ed98041744c520d52ac4a92421694
Author: ChrisChinchilla <chris@chronosphere.io>
Date:   Mon Oct 5 12:53:34 2020 +0200

    Add redirects

    Signed-off-by: ChrisChinchilla <chris@chronosphere.io>

commit 612cfbb319b30177501530f9cd7bdd626ea6b107
Author: ChrisChinchilla <chris@chronosphere.io>
Date:   Mon Oct 5 12:09:40 2020 +0200

    Remove versioning for now

    Signed-off-by: ChrisChinchilla <chris@chronosphere.io>

commit cda48e18dce956a4e684dd4d6d7943df808e6846
Author: ChrisChinchilla <chris@chronosphere.io>
Date:   Fri Oct 2 18:09:35 2020 +0200

    Switch netlify directory

    Signed-off-by: ChrisChinchilla <chris@chronosphere.io>

commit c5d0b1decc95d042a565cbe97d25c9eda94549a9
Author: ChrisChinchilla <chris@chronosphere.io>
Date:   Fri Oct 2 18:00:34 2020 +0200

    Move theme to module

    Signed-off-by: ChrisChinchilla <chris@chronosphere.io>

commit 855d8c7af1bf45978ed180469ecb1b753ec1abdb
Author: ChrisChinchilla <chris@chronosphere.io>
Date:   Thu Oct 1 14:59:14 2020 +0200

    Netlify dev

    Signed-off-by: ChrisChinchilla <chris@chronosphere.io>

commit a8a8eda0f2f3b8fa1b3aefa2316942ff692dc8e5
Author: ChrisChinchilla <chris@chronosphere.io>
Date:   Thu Oct 1 14:03:34 2020 +0200

    Update Hugo version

    Signed-off-by: ChrisChinchilla <chris@chronosphere.io>

commit f19ffadb7caad18663dad99ec5230e8357125954
Author: ChrisChinchilla <chris@chronosphere.io>
Date:   Thu Oct 1 13:54:41 2020 +0200

    Random file to fix odd git issues

    Signed-off-by: ChrisChinchilla <chris@chronosphere.io>

commit a6413db7891f2f741510e4b36300e3cb93904f1d
Author: ChrisChinchilla <chris@chronosphere.io>
Date:   Thu Oct 1 13:11:42 2020 +0200

    Update versions

    Signed-off-by: ChrisChinchilla <chris@chronosphere.io>

commit 9919f004633ace0f88bb9cb721a6373416f28781
Author: ChrisChinchilla <chris@chronosphere.io>
Date:   Thu Oct 1 12:45:25 2020 +0200

    Convert docs theme to module

    Signed-off-by: ChrisChinchilla <chris@chronosphere.io>

commit 3a1e013338b6671eb601df3f5cbdd89d770ca171
Author: ChrisChinchilla <chris@chronosphere.io>
Date:   Thu Oct 1 12:27:31 2020 +0200

    Remove subtree again, not working

    Signed-off-by: ChrisChinchilla <chris@chronosphere.io>

commit 282ca2d870eb656ba15e20ade3d8aec6493523fe
Author: ChrisChinchilla <chris@chronosphere.io>
Date:   Thu Oct 1 12:20:11 2020 +0200

    Add versions to config

    Signed-off-by: ChrisChinchilla <chris@chronosphere.io>

commit 73f03d34990b740d5bb1c57df1c5fbb328d0f701
Author: ChrisChinchilla <chris@chronosphere.io>
Date:   Wed Sep 30 18:29:35 2020 +0200

    Consolidate all commits

commit f2ebf5c
Author: teddywahle <69990143+teddywahle@users.noreply.github.com>
Date:   Mon Sep 21 12:33:37 2020 -0700

    [query] Implemented the Graphite `integralByInterval` function  (#2596)

commit a66fb7d
Author: arnikola <artem@chronosphere.io>
Date:   Mon Sep 21 14:33:57 2020 -0400

    [dbnode] Tile iterators for wide aggregations (#2646)

commit 9ea5682
Author: teddywahle <69990143+teddywahle@users.noreply.github.com>
Date:   Sun Sep 20 21:50:31 2020 -0700

    [query] Implemented the Graphite `divideSeriesLists` function (#2585)

commit 35cac59
Author: Rob Skillington <rob.skillington@gmail.com>
Date:   Mon Sep 21 00:21:58 2020 -0400

    [coordinator] Update OpenAPI specs for namespace update endpoint (#2629)

commit ef83ec4
Author: Rob Skillington <rob.skillington@gmail.com>
Date:   Sun Sep 20 21:57:26 2020 -0400

    [changelog] Add changelog for 0.15.15 (#2649)

commit 091f833
Author: Rob Skillington <rob.skillington@gmail.com>
Date:   Fri Sep 18 11:30:57 2020 -0400

    [coordinator] Allow configuration of tag validation (#2647)

commit 3476b4e
Author: Gediminas Guoba <gediminas@chronosphere.io>
Date:   Fri Sep 18 17:24:09 2020 +0300

    [dbnode] Streaming writer (#2618)

    * [dbnode] Large tiles writer

    * minor refactorings

    * minor refactoring

    * Skip tagsEncoder, use encodedTags directly

    * Rename LargeTilesWriter to StreamingWriter

    * Add FIXME wrt stegment.Tail.Finalize

    * Address PR feedback

    Co-authored-by: Linas Medziunas <linas.medziunas@gmail.com>
    Co-authored-by: Linas Medžiūnas <linasm@users.noreply.github.com>

commit 88164cf
Author: Ryan Hall <ryanhall07@gmail.com>
Date:   Thu Sep 17 16:38:04 2020 -0700

    Only read the commit log once during bootstrapping (#2645)

    * Only read the commit log once during bootstrapping

    A recent refactoring of cold writes ( #2508) introduced a regression that increases the
    chances the commit log is read twice while bootstrapping. The referenced PR changed the commitlog
    bootstrapper to read all requested time ranges, even if a range had been fulfilled by a previous
    bootstrapper. This was necessary since the commitlog may have cold writes that were never commmited
    to a fileset. The fileystem bootstrapper would report a time range as fulfilled, but might be missing
    cold writes only in the commit log.

    It should be noted this bug was always theoretically possible, but unlikely since the commitlog bootstrapper
    typically wouldn't run in the first pass (cold time ranges) since the filesystem would fulfill all cold ranges
    and short circuit the first pass of the boostrapper.

    This change only reads the commit log on the first pass of the boostrapper and caches the result to skip reading
    it in subsequent passes. It doesn't actually matter which pass we read the commit log, the first was just chosen
    arbitrarily.

    Other attempts at fixing this bug attempted to disable the entire commit log bootstrapper during a pass, but that's
    not possible since the commit log bootstrapper is actually 2 bootstrappers in one, both the the commit log and snapshot
    files. To minimize the refactoring changes we still want to only read the snapshot files of the requested ranges.

commit 3d2915f
Author: nate <nbroyles@gmail.com>
Date:   Thu Sep 17 13:03:48 2020 -0400

    [dbnode] Make caching after block retrieval a configuration option (#2613)

commit 0ef7aba
Author: nate <nbroyles@gmail.com>
Date:   Thu Sep 17 12:33:53 2020 -0400

    [docs] Add documentation on fileset migrations (#2630)

Signed-off-by: ChrisChinchilla <chris@chronosphere.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@arnikola arnikola arnikola approved these changes

@robskillington robskillington Awaiting requested review from robskillington

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@teddywahle @robskillington @arnikola