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

Start switching to use slog #654

Merged
merged 1 commit into from
Oct 6, 2023
Merged

Start switching to use slog #654

merged 1 commit into from
Oct 6, 2023

Conversation

norkans7
Copy link
Contributor

@norkans7 norkans7 commented Oct 6, 2023

No description provided.

@norkans7 norkans7 marked this pull request as ready for review October 6, 2023 10:27
@norkans7 norkans7 requested a review from rowanseymour October 6, 2023 10:27
@codecov
Copy link

codecov bot commented Oct 6, 2023

Codecov Report

Merging #654 (e53e137) into main (ccab456) will decrease coverage by 0.27%.
The diff coverage is 3.77%.

@@            Coverage Diff             @@
##             main     #654      +/-   ##
==========================================
- Coverage   74.32%   74.05%   -0.27%     
==========================================
  Files          99      100       +1     
  Lines       13136    13183      +47     
==========================================
  Hits         9763     9763              
- Misses       2685     2732      +47     
  Partials      688      688              
Files Coverage Δ
server.go 77.63% <100.00%> (ø)
handlers/test.go 0.00% <0.00%> (ø)
queue/queue.go 80.55% <0.00%> (ø)
utils/logrus.go 0.00% <0.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@@ -20,6 +20,7 @@ import (
"github.com/nyaruka/gocommon/urns"
"github.com/sirupsen/logrus"
"github.com/stretchr/testify/assert"
"golang.org/x/exp/slog"
Copy link
Member

Choose a reason for hiding this comment

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

"log/slog"

@@ -20,6 +20,7 @@ import (
"github.com/nyaruka/gocommon/urns"
"github.com/sirupsen/logrus"
"github.com/stretchr/testify/assert"
"golang.org/x/exp/slog"
Copy link
Member

Choose a reason for hiding this comment

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

"log/slog"

queue/queue.go Outdated
@@ -6,7 +6,7 @@ import (
"time"

"github.com/gomodule/redigo/redis"
"github.com/sirupsen/logrus"
"golang.org/x/exp/slog"
Copy link
Member

Choose a reason for hiding this comment

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

"log/slog"

server.go Outdated
@@ -22,6 +22,7 @@ import (
"github.com/nyaruka/gocommon/jsonx"
"github.com/pkg/errors"
"github.com/sirupsen/logrus"
"golang.org/x/exp/slog"
Copy link
Member

Choose a reason for hiding this comment

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

"log/slog"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

replaced properly

Copy link
Member

Choose a reason for hiding this comment

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

the golang.org/x/exp is where new experimental features are tried out before sometimes becoming part of the golang std lib - so slog first existed there before being added to go 1.21

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, I had not noticed the completion chose that one instead of the std lib one, I guess I have some package not up to date

"os"
"os/signal"
goruntime "runtime"
Copy link
Member

Choose a reason for hiding this comment

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

this is only necessary in mailroom because we have our own runtime struct

Copy link
Contributor Author

Choose a reason for hiding this comment

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

reverted that

@norkans7 norkans7 force-pushed the slog branch 2 times, most recently from be4a311 to 64470aa Compare October 6, 2023 14:24
@@ -13,6 +13,8 @@ import (
"testing"
"time"

"log/slog"
Copy link
Member

Choose a reason for hiding this comment

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

no space needed between this and other deps - standard go formatting is only a space between std lib deps and other deps

@@ -25,6 +25,7 @@ github.com/getsentry/raven-go v0.2.0/go.mod h1:KungGk8q33+aIAZUIVWZDr2OfAEBsO49P
github.com/go-chi/chi v4.1.2+incompatible h1:fGFk2Gmi/YKXk0OmGfBh0WgmN3XB8lVnEyNz34tQRec=
github.com/go-chi/chi v4.1.2+incompatible/go.mod h1:eB3wogJHnLi3x/kFX2A+IbTBlXxmMeXJVKy9tTv1XzQ=
github.com/go-playground/assert/v2 v2.2.0 h1:JvknZsQTYeFEAhQwI4qEt9cyV5ONwRHC+lYKSsYSR8s=
github.com/go-playground/assert/v2 v2.2.0/go.mod h1:VDjEfimB/XKnb+ZQfWdccd7VUvScMdVu0Titje2rxJ4=
Copy link
Member

Choose a reason for hiding this comment

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

go mod tidy

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes

queue/queue.go Outdated
@@ -201,7 +201,7 @@ func PopFromQueue(conn redis.Conn, qType string) (WorkerToken, string, error) {
epochMS := strconv.FormatFloat(float64(time.Now().UnixNano()/int64(time.Microsecond))/float64(1000000), 'f', 6, 64)
values, err := redis.Strings(luaPop.Do(conn, epochMS, qType))
if err != nil {
logrus.Error(err)
slog.Error("error poping from queue", "error", err)
Copy link
Member

Choose a reason for hiding this comment

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

popping

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@rowanseymour rowanseymour merged commit 7970c3f into main Oct 6, 2023
5 of 7 checks passed
@rowanseymour rowanseymour deleted the slog branch October 6, 2023 16:24
@github-actions github-actions bot locked and limited conversation to collaborators Oct 6, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants