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

Chore: Copy cortex/util/math into Loki #5036

Merged
merged 6 commits into from
Jan 5, 2022
Merged

Chore: Copy cortex/util/math into Loki #5036

merged 6 commits into from
Jan 5, 2022

Conversation

kavirajk
Copy link
Contributor

@kavirajk kavirajk commented Jan 4, 2022

What this PR does / why we need it:
Copy the cortex/util/math dependency into Loki.
Also moved some helpers from util/math.go -> util/math package.

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:

Checklist

  • Documentation added
  • Tests updated
  • Add an entry in the CHANGELOG.md about the changes.

Signed-off-by: Kaviraj <kavirajkanagaraj@gmail.com>
Signed-off-by: Kaviraj <kavirajkanagaraj@gmail.com>
Signed-off-by: Kaviraj <kavirajkanagaraj@gmail.com>
@kavirajk kavirajk marked this pull request as ready for review January 4, 2022 20:57
@kavirajk kavirajk requested a review from a team as a code owner January 4, 2022 20:57
@kavirajk kavirajk requested a review from DylanGuedes January 4, 2022 20:58
init bool
}

func NewEWMARate(alpha float64, interval time.Duration) *EwmaRate {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this used somewhere ? Not sure it is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea you are right. Removed it.

Signed-off-by: Kaviraj <kavirajkanagaraj@gmail.com>
@pull-request-size pull-request-size bot added size/M and removed size/L labels Jan 5, 2022
Copy link
Contributor

@cyriltovena cyriltovena left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@sandeepsukhani sandeepsukhani left a comment

Choose a reason for hiding this comment

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

There are some unwanted spaces under imports sections so can you please fix it?

Comment on lines 33 to 36
"github.com/grafana/loki/pkg/util/math"

"github.com/grafana/loki/pkg/util/spanlogger"

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please remove unwanted spaces here?
Same for some of the other files updated in this PR.

@kavirajk kavirajk changed the title No cortex util math Chore: Copy cortex/util/math into Loki Jan 5, 2022
Signed-off-by: Kaviraj <kavirajkanagaraj@gmail.com>
Copy link
Contributor

@sandeepsukhani sandeepsukhani left a comment

Choose a reason for hiding this comment

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

last 4 changes required, I promise 😬

@@ -9,7 +9,7 @@ import (
otlog "github.com/opentracing/opentracing-go/log"
"github.com/pkg/errors"

"github.com/cortexproject/cortex/pkg/util/math"
"github.com/grafana/loki/pkg/util/math"

Copy link
Contributor

Choose a reason for hiding this comment

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

One more here. Sorry for being picky, we would miss fixing it until we start working on it again.

@@ -9,7 +9,7 @@ import (

ot "github.com/opentracing/opentracing-go"

"github.com/cortexproject/cortex/pkg/util/math"
"github.com/grafana/loki/pkg/util/math"

Copy link
Contributor

Choose a reason for hiding this comment

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

same here

@@ -7,7 +7,7 @@ import (
"github.com/stretchr/testify/require"
"google.golang.org/grpc"

util_math "github.com/cortexproject/cortex/pkg/util/math"
util_math "github.com/grafana/loki/pkg/util/math"

Copy link
Contributor

Choose a reason for hiding this comment

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

same here

@@ -5,7 +5,7 @@ import (
"sync"
"unsafe"

util_math "github.com/cortexproject/cortex/pkg/util/math"
util_math "github.com/grafana/loki/pkg/util/math"

Copy link
Contributor

Choose a reason for hiding this comment

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

same here

Signed-off-by: Kaviraj <kavirajkanagaraj@gmail.com>
@kavirajk
Copy link
Contributor Author

kavirajk commented Jan 5, 2022

Fixed the spacing issue @sandeepsukhani. Thanks for the catch :) (thought goimports -local "github.com/grafana/loki" should have grouped correctly with removing spaces between those imports. But apparently didn't.

Copy link
Contributor

@sandeepsukhani sandeepsukhani left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks Kavi for taking care of changes!

@sandeepsukhani sandeepsukhani merged commit 14afb13 into grafana:main Jan 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants