Skip to content

Commit

Permalink
refactor: inject logger from the top in the required packages (#66)
Browse files Browse the repository at this point in the history 
* refactor: inject logger from the top in the api package

* refactor: inject logger in the required packages

* chore: update go modules

* chore: clean up

* fix: resolve conflicts
  • Loading branch information
@scortier
scortier authored Jan 24, 2022
1 parent da942bb commit 6eaf424
Show file tree
Hide file tree
Showing 21 changed files with 152 additions and 206 deletions.
11 changes: 5 additions & 6 deletions api/handlers/json.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,14 +3,13 @@ package handlers
import (
"encoding/json"
"fmt"
"log"
"github.com/odpf/salt/log"
"net/http"
"time"

"github.com/sirupsen/logrus"
)

func writeJSON(w http.ResponseWriter, status int, v interface{}) {
logger := log.NewLogrus()
w.Header().Set("content-type", "application/json")
w.WriteHeader(status)
err := json.NewEncoder(w).Encode(v)
Expand All @@ -19,15 +18,15 @@ func writeJSON(w http.ResponseWriter, status int, v interface{}) {
w.WriteHeader(status)
code, err := w.Write([]byte("error encoding response to json"))
if err != nil {
log.Print(fmt.Sprintf("error writing response with code: %d", code))
logger.Info(fmt.Sprintf("error writing response with code: %d", code))
}
}
}

func internalServerError(w http.ResponseWriter, logger logrus.FieldLogger, msg string) {
func internalServerError(w http.ResponseWriter, logger log.Logger, msg string) {
ref := time.Now().Unix()

logger.Errorf("ref (%d): %s", ref, msg)
logger.Error("time format", "ref", ref, "msg", msg)
response := &ErrorResponse{
Reason: fmt.Sprintf(
"%s - ref (%d)",
Expand Down
24 changes: 8 additions & 16 deletions api/handlers/lineage_handler.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,14 @@ package handlers

import (
"fmt"
"github.com/odpf/salt/log"
"net/http"
"net/url"
"strconv"

"github.com/gorilla/mux"
"github.com/odpf/columbus/lineage"
"github.com/odpf/columbus/record"
"github.com/sirupsen/logrus"
)

// interface to lineage.Service
Expand All @@ -19,13 +19,13 @@ type LineageProvider interface {
}

type LineageHandler struct {
log logrus.FieldLogger
logger log.Logger
lineageProvider LineageProvider
}

func NewLineageHandler(log logrus.FieldLogger, provider LineageProvider) *LineageHandler {
func NewLineageHandler(logger log.Logger, provider LineageProvider) *LineageHandler {
handler := &LineageHandler{
log: log,
logger: logger,
lineageProvider: provider,
}

Expand All @@ -35,9 +35,7 @@ func NewLineageHandler(log logrus.FieldLogger, provider LineageProvider) *Lineag
func (handler *LineageHandler) ListLineage(w http.ResponseWriter, r *http.Request) {
graph, err := handler.lineageProvider.Graph()
if err != nil {
handler.log.
Errorf("error requesting graph: %v", err)

handler.logger.Error("error requesting graph", "error", err)
status := http.StatusInternalServerError
writeJSONError(w, status, http.StatusText(status))
return
Expand All @@ -46,10 +44,7 @@ func (handler *LineageHandler) ListLineage(w http.ResponseWriter, r *http.Reques
opts := handler.parseOpts(r.URL.Query())
res, err := graph.Query(opts)
if err != nil {
handler.log.
WithField("query", opts).
Errorf("error querying graph: %v\ncfg: %v", err, opts)

handler.logger.Error("error querying graph", "query", opts, "error", err)
status := http.StatusBadRequest
if _, ok := err.(record.ErrNoSuchType); ok {
status = http.StatusNotFound
Expand All @@ -64,7 +59,7 @@ func (handler *LineageHandler) ListLineage(w http.ResponseWriter, r *http.Reques
func (handler *LineageHandler) GetLineage(w http.ResponseWriter, r *http.Request) {
graph, err := handler.lineageProvider.Graph()
if err != nil {
handler.log.Errorf("error requesting graph: %v", err)
handler.logger.Error("error requesting graph", "error", err)
status := http.StatusInternalServerError
writeJSONError(w, status, http.StatusText(status))
return
Expand All @@ -76,10 +71,7 @@ func (handler *LineageHandler) GetLineage(w http.ResponseWriter, r *http.Request

res, err := graph.Query(opts)
if err != nil {
handler.log.
WithField("query", opts).
Errorf("error querying graph: %v\ncfg: %v", err, opts)

handler.logger.Error("error querying graph", "query", opts, "error", err)
status := http.StatusBadRequest
if _, ok := err.(record.ErrNoSuchType); ok {
status = http.StatusNotFound
Expand Down
12 changes: 7 additions & 5 deletions api/handlers/lineage_handler_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package handlers_test
import (
"encoding/json"
"errors"
"github.com/odpf/salt/log"
"net/http"
"net/http/httptest"
"reflect"
Expand All @@ -17,6 +18,7 @@ import (
)

func TestLineageHandler(t *testing.T) {
logger := log.NewNoop()
t.Run("ListLineage", func(t *testing.T) {
t.Run("should return 404 if a non-existent type is requested", func(t *testing.T) {
graph := new(mock.Graph)
Expand All @@ -27,7 +29,7 @@ func TestLineageHandler(t *testing.T) {
lp := new(mock.LineageProvider)
lp.On("Graph").Return(graph, nil)

handler := handlers.NewLineageHandler(new(mock.Logger), lp)
handler := handlers.NewLineageHandler(logger, lp)
rr := httptest.NewRequest("GET", "/?filter.type=bqtable", nil)
rw := httptest.NewRecorder()
handler.ListLineage(rw, rr)
Expand Down Expand Up @@ -71,7 +73,7 @@ func TestLineageHandler(t *testing.T) {
lp := new(mock.LineageProvider)
lp.On("Graph").Return(graph, nil)

handler := handlers.NewLineageHandler(new(mock.Logger), lp)
handler := handlers.NewLineageHandler(logger, lp)

rr := httptest.NewRequest("GET", "/?filter.type=topic&filter.type=table", nil)
rw := httptest.NewRecorder()
Expand Down Expand Up @@ -101,7 +103,7 @@ func TestLineageHandler(t *testing.T) {
lp := new(mock.LineageProvider)
lp.On("Graph").Return(graph, errNoGraph)

handler := handlers.NewLineageHandler(new(mock.Logger), lp)
handler := handlers.NewLineageHandler(logger, lp)

rr := httptest.NewRequest("GET", "/", nil)
rw := httptest.NewRecorder()
Expand Down Expand Up @@ -134,7 +136,7 @@ func TestLineageHandler(t *testing.T) {
lp := new(mock.LineageProvider)
lp.On("Graph").Return(graph, nil)

handler := handlers.NewLineageHandler(new(mock.Logger), lp)
handler := handlers.NewLineageHandler(logger, lp)

rr := httptest.NewRequest("GET", "/", nil)
rw := httptest.NewRecorder()
Expand Down Expand Up @@ -187,7 +189,7 @@ func TestLineageHandler(t *testing.T) {
lp := new(mock.LineageProvider)
lp.On("Graph").Return(graph, nil)

handler := handlers.NewLineageHandler(new(mock.Logger), lp)
handler := handlers.NewLineageHandler(logger, lp)

rr := httptest.NewRequest("GET", "/v1/lineage/table/raw", nil)
rw := httptest.NewRecorder()
Expand Down
36 changes: 12 additions & 24 deletions api/handlers/record_handler.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package handlers
import (
"encoding/json"
"fmt"
"github.com/odpf/salt/log"
"net/http"
"net/url"
"strconv"
Expand All @@ -11,19 +12,18 @@ import (
"github.com/gorilla/mux"
"github.com/odpf/columbus/discovery"
"github.com/odpf/columbus/record"
"github.com/sirupsen/logrus"
)

// RecordHandler exposes a REST interface to types
type RecordHandler struct {
typeRepository record.TypeRepository
recordRepositoryFactory discovery.RecordRepositoryFactory
discoveryService *discovery.Service
logger logrus.FieldLogger
logger log.Logger
}

func NewRecordHandler(
logger logrus.FieldLogger,
logger log.Logger,
typeRepository record.TypeRepository,
discoveryService *discovery.Service,
rrf discovery.RecordRepositoryFactory) *RecordHandler {
Expand Down Expand Up @@ -54,8 +54,7 @@ func (h *RecordHandler) Delete(w http.ResponseWriter, r *http.Request) {

err := h.discoveryService.DeleteRecord(r.Context(), typName.String(), recordID)
if err != nil {
h.logger.
Errorf("error deleting record \"%s\": %v", typName, err)
h.logger.Error("error deleting record", "type", typName, "error", err)

if _, ok := err.(record.ErrNoSuchRecord); ok {
statusCode = http.StatusNotFound
Expand All @@ -66,7 +65,7 @@ func (h *RecordHandler) Delete(w http.ResponseWriter, r *http.Request) {
return
}

h.logger.Infof("deleted record \"%s\" with type \"%s\"", recordID, typName)
h.logger.Info("deleted record", "record id", recordID, "type", typName)
writeJSON(w, http.StatusNoContent, "success")
}

Expand All @@ -89,9 +88,7 @@ func (h *RecordHandler) UpsertBulk(w http.ResponseWriter, r *http.Request) {
var failedRecords = make(map[int]string)
for idx, record := range records {
if err := h.validateRecord(record); err != nil {
h.logger.WithField("type", typName).
WithField("record", record).
Errorf("error validating record: %v", err)
h.logger.Error("error validating record", "type", typName, "record", record, "error", err)
failedRecords[idx] = err.Error()
}
}
Expand All @@ -101,14 +98,12 @@ func (h *RecordHandler) UpsertBulk(w http.ResponseWriter, r *http.Request) {
}

if err := h.discoveryService.Upsert(r.Context(), typName.String(), records); err != nil {
h.logger.WithField("type", typName).
Errorf("error creating/updating records: %v", err)

h.logger.Error("error creating/updating records", "type", typName, "error", err)
status := http.StatusInternalServerError
writeJSONError(w, status, http.StatusText(status))
return
}
h.logger.Infof("created/updated %d records for %q type", len(records), typName)
h.logger.Info("created/updated records", "record count", len(records), "type", typName)
writeJSON(w, http.StatusOK, StatusResponse{Status: "success"})
}

Expand All @@ -123,8 +118,7 @@ func (h *RecordHandler) GetByType(w http.ResponseWriter, r *http.Request) {

recordRepo, err := h.recordRepositoryFactory.For(typName.String())
if err != nil {
h.logger.WithField("type", typName).
Errorf("error constructing record repository: %v", err)
h.logger.Error("error constructing record repository", "type", typName, "error", err)
status, message := h.responseStatusForError(err)
writeJSONError(w, status, message)
return
Expand All @@ -137,8 +131,7 @@ func (h *RecordHandler) GetByType(w http.ResponseWriter, r *http.Request) {

recordList, err := recordRepo.GetAll(r.Context(), getCfg)
if err != nil {
h.logger.WithField("type", typName).
Errorf("error fetching records: GetAll: %v", err)
h.logger.Error("error fetching records: GetAll", "type", typName, "error", err)
status, message := h.responseStatusForError(err)
writeJSONError(w, status, message)
return
Expand Down Expand Up @@ -166,20 +159,15 @@ func (h *RecordHandler) GetOneByType(w http.ResponseWriter, r *http.Request) {

recordRepo, err := h.recordRepositoryFactory.For(typName.String())
if err != nil {
h.logger.WithField("type", typName).
Errorf("internal: error construing record repository: %v", err)

h.logger.Error("internal: error construing record repository", "type", typName, "error", err)
status := http.StatusInternalServerError
writeJSONError(w, status, http.StatusText(status))
return
}

record, err := recordRepo.GetByID(r.Context(), recordID)
if err != nil {
h.logger.WithField("type", typName).
WithField("record", recordID).
Errorf("error fetching record: %v", err)

h.logger.Error("error fetching record", "type", typName, "record id", recordID, "error", err)
status, message := h.responseStatusForError(err)
writeJSONError(w, status, message)
return
Expand Down
18 changes: 10 additions & 8 deletions api/handlers/record_handler_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"encoding/json"
"errors"
"fmt"
"github.com/odpf/salt/log"
"net/http"
"net/http/httptest"
"net/url"
Expand All @@ -24,6 +25,7 @@ func TestRecordHandler(t *testing.T) {
var (
ctx = tmock.AnythingOfType("*context.valueCtx")
typeName = record.TypeNameTable.String()
logger = log.NewNoop()
)

tr := new(mock.TypeRepository)
Expand Down Expand Up @@ -59,7 +61,7 @@ func TestRecordHandler(t *testing.T) {
"name": typeName,
})

handler := handlers.NewRecordHandler(new(mock.Logger), tr, nil, nil)
handler := handlers.NewRecordHandler(logger, tr, nil, nil)
handler.UpsertBulk(rw, rr)

expectedStatus := http.StatusBadRequest
Expand All @@ -77,7 +79,7 @@ func TestRecordHandler(t *testing.T) {
"name": "invalid",
})

handler := handlers.NewRecordHandler(new(mock.Logger), tr, nil, nil)
handler := handlers.NewRecordHandler(logger, tr, nil, nil)
handler.UpsertBulk(rw, rr)

expectedStatus := http.StatusNotFound
Expand Down Expand Up @@ -107,7 +109,7 @@ func TestRecordHandler(t *testing.T) {
defer recordRepoFac.AssertExpectations(t)

service := discovery.NewService(recordRepoFac, nil)
handler := handlers.NewRecordHandler(new(mock.Logger), tr, service, recordRepoFac)
handler := handlers.NewRecordHandler(logger, tr, service, recordRepoFac)
handler.UpsertBulk(rw, rr)

expectedStatus := http.StatusInternalServerError
Expand Down Expand Up @@ -152,7 +154,7 @@ func TestRecordHandler(t *testing.T) {
defer recordRepoFac.AssertExpectations(t)

service := discovery.NewService(recordRepoFac, nil)
handler := handlers.NewRecordHandler(new(mock.Logger), tr, service, recordRepoFac)
handler := handlers.NewRecordHandler(logger, tr, service, recordRepoFac)
handler.UpsertBulk(rw, rr)

expectedStatus := http.StatusInternalServerError
Expand Down Expand Up @@ -196,7 +198,7 @@ func TestRecordHandler(t *testing.T) {
defer recordRepoFac.AssertExpectations(t)

service := discovery.NewService(recordRepoFac, nil)
handler := handlers.NewRecordHandler(new(mock.Logger), tr, service, recordRepoFac)
handler := handlers.NewRecordHandler(logger, tr, service, recordRepoFac)
handler.UpsertBulk(rw, rr)

expectedStatus := http.StatusOK
Expand Down Expand Up @@ -285,7 +287,7 @@ func TestRecordHandler(t *testing.T) {
defer recordRepo.AssertExpectations(t)

service := discovery.NewService(recordRepoFactory, nil)
handler := handlers.NewRecordHandler(new(mock.Logger), tr, service, recordRepoFactory)
handler := handlers.NewRecordHandler(logger, tr, service, recordRepoFactory)
handler.Delete(rw, rr)

if rw.Code != tc.ExpectStatus {
Expand Down Expand Up @@ -475,7 +477,7 @@ func TestRecordHandler(t *testing.T) {
tc.Setup(&tc, rrf)

service := discovery.NewService(rrf, nil)
handler := handlers.NewRecordHandler(new(mock.Logger), tr, service, rrf)
handler := handlers.NewRecordHandler(logger, tr, service, rrf)
handler.GetByType(rw, rr)

if rw.Code != tc.ExpectStatus {
Expand Down Expand Up @@ -572,7 +574,7 @@ func TestRecordHandler(t *testing.T) {
}

service := discovery.NewService(recordRepoFac, nil)
handler := handlers.NewRecordHandler(new(mock.Logger), tr, service, recordRepoFac)
handler := handlers.NewRecordHandler(logger, tr, service, recordRepoFac)
handler.GetOneByType(rw, rr)

if rw.Code != tc.ExpectStatus {
Expand Down
Loading

0 comments on commit 6eaf424

Please sign in to comment.