Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 8 additions & 6 deletions cmd/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,13 +19,15 @@ import (
"flag"
"fmt"
"log"
"log/slog"
"net"
"net/http"
_ "net/http/pprof"
"os"
"runtime"
"runtime/pprof"

internallog "github.com/datacommonsorg/mixer/internal/log"
"github.com/datacommonsorg/mixer/internal/metrics"
pbs "github.com/datacommonsorg/mixer/internal/proto/service"
"github.com/datacommonsorg/mixer/internal/server"
Expand Down Expand Up @@ -113,10 +115,10 @@ var (
)

func main() {
log.Println("Enter mixer main() function")
internallog.SetUpLogger()
slog.Info("Enter mixer main() function")
// Parse flag
flag.Parse()
log.SetFlags(log.LstdFlags | log.Lshortfile)

if *v3MirrorFraction < 0 || *v3MirrorFraction > 1.0 {
log.Fatalf("v3_mirror_fraction must be between 0 and 1.0, got %f", *v3MirrorFraction)
Expand All @@ -136,7 +138,7 @@ func main() {
}
err := profiler.Start(cfg)
if err != nil {
log.Printf("Failed to start profiler: %v", err)
slog.Warn("Failed to start profiler", "error", err)
}
}

Expand Down Expand Up @@ -286,7 +288,7 @@ func main() {

if *useCloudSQL {
if sqldb.IsConnected(&sqlClient) {
log.Printf("SQL client has already been created, will not use CloudSQL")
slog.Warn("SQL client has already been created, will not use CloudSQL")
} else {
client, err := sqldb.NewCloudSQLClient(*cloudSQLInstance)
if err != nil {
Expand Down Expand Up @@ -417,7 +419,7 @@ func main() {
go func() {
// Code from https://pkg.go.dev/net/http/pprof README
httpProfileFrom := fmt.Sprintf("localhost:%d", *httpProfilePort)
log.Printf("Serving profile over HTTP on %v", httpProfileFrom)
slog.Info("Serving profile over HTTP", "address", httpProfileFrom)
log.Printf("%s\n", http.ListenAndServe(httpProfileFrom, nil))
}()
}
Expand All @@ -427,7 +429,7 @@ func main() {
if err != nil {
log.Fatalf("Failed to listen on network: %v", err)
}
log.Println("Mixer ready to serve!!")
slog.Info("Mixer ready to serve!!")
if err := srv.Serve(lis); err != nil {
log.Fatalf("Failed to serve: %v", err)
}
Expand Down
12 changes: 12 additions & 0 deletions internal/log/log.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
package log

import (
"log/slog"
"os"
)

// SetUpLogger sets up a logger.
func SetUpLogger() {
logger := slog.New(slog.NewJSONHandler(os.Stdout, &slog.HandlerOptions{AddSource: true}))
slog.SetDefault(logger)
}
4 changes: 2 additions & 2 deletions internal/server/v2/facet/contained_in.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ package facet

import (
"context"
"log"
"log/slog"
"net/http"
"sort"

Expand Down Expand Up @@ -172,7 +172,7 @@ func btContainedInFacet(
totalSeries,
)
}
log.Println("Fetch series cache in contained-in observation query")
slog.Info("Fetch series cache in contained-in observation query")
// When date doesn't matter, use SeriesFacet to get the facets for the
// child places
if queryDate == "" || queryDate == shared.LATEST {
Expand Down
4 changes: 2 additions & 2 deletions internal/server/v2/observation/contained_in.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ package observation

import (
"context"
"log"
"log/slog"
"net/http"
"sort"

Expand Down Expand Up @@ -143,7 +143,7 @@ func FetchContainedIn(
totalSeries,
)
}
log.Println("Fetch series cache in contained-in observation query")
slog.Info("Fetch series cache in contained-in observation query")
directResp, err := FetchDirectBT(
ctx,
store.BtGroup,
Expand Down
5 changes: 3 additions & 2 deletions test/http_memprof/http_memprof.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import (
"flag"
"fmt"
"log"
"log/slog"
"os"
"os/exec"
"path/filepath"
Expand Down Expand Up @@ -188,7 +189,7 @@ func writeResultsToCsv(results []*MemoryProfileResult, outputPath string) {
}

func main() {
log.SetFlags(log.LstdFlags | log.Lshortfile)
slog.SetDefault(slog.New(slog.NewJSONHandler(os.Stdout, nil)))

flag.Parse()

Expand Down Expand Up @@ -229,7 +230,7 @@ func main() {
log.Fatalf("Could not connect to %s: timed out. Last state: %s", *grpcAddr, s)
}
}
log.Println("Connected to gRPC succesfully")
slog.Info("Connected to gRPC succesfully")

//nolint:errcheck // TODO: Fix pre-existing issue and remove comment.
defer conn.Close()
Expand Down
13 changes: 7 additions & 6 deletions test/setup.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import (
"context"
"encoding/json"
"log"
"log/slog"
"net"
"os"
"path"
Expand Down Expand Up @@ -298,11 +299,11 @@ func UpdateGolden(v interface{}, root, fname string) {
encoder.SetIndent("", " ")
err := encoder.Encode(v)
if err != nil {
log.Printf("could not encode golden response %v", err)
slog.Warn("could not encode golden response", "err", err)
}
if os.WriteFile(
path.Join(root, fname), bytes.TrimRight(buf.Bytes(), "\n"), 0644) != nil {
log.Printf("could not write golden files to %s", fname)
slog.Warn("could not write golden files to", "err", err)
Copy link
Contributor

Choose a reason for hiding this comment

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

high

This log is misleading. It's logging the err variable from the encoder.Encode call on line 300, not the error from os.WriteFile. The error from os.WriteFile is not being captured. The surrounding if block should be changed to capture and log the correct error, for example:

if err := os.WriteFile(
        path.Join(root, fname), bytes.TrimRight(buf.Bytes(), "\n"), 0644); err != nil {
    slog.Warn("could not write golden file", "file", fname, "err", err)
}

}
}

Expand All @@ -316,18 +317,18 @@ func UpdateProtoGolden(
// Use encoding/json to get stable output.
data, err := marshaller.Marshal(resp)
if err != nil {
log.Printf("marshaller.Marshal(%s) = %s", fname, err)
slog.Warn("marshaller.Marshal()", "err", err)
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

The new log message loses the context of which file (fname) failed. It's good practice to include such contextual information in structured logs for easier debugging.

Suggested change
slog.Warn("marshaller.Marshal()", "err", err)
slog.Warn("marshaller.Marshal() failed", "file", fname, "err", err)

return
}
var rm json.RawMessage = data
jsonByte, err := json.MarshalIndent(rm, "", " ")
if err != nil {
log.Printf("json.MarshalIndent(%s) = %s", fname, err)
slog.Warn("json.MarshalIndent()", "err", err)
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

Similar to a previous comment, the filename context (fname) is lost in this log message. Including it would make debugging easier.

Suggested change
slog.Warn("json.MarshalIndent()", "err", err)
slog.Warn("json.MarshalIndent() failed", "file", fname, "err", err)

return
}
err = os.WriteFile(path.Join(root, fname), jsonByte, 0644)
if err != nil {
log.Printf("os.WriteFile(%s) = %s", fname, err)
slog.Warn("os.WriteFile", "err", err)
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

The filename context (fname) is lost here as well. Please add it to the structured log to provide more context for debugging.

Suggested change
slog.Warn("os.WriteFile", "err", err)
slog.Warn("os.WriteFile failed", "file", fname, "err", err)

}
}

Expand Down Expand Up @@ -377,7 +378,7 @@ func ReadGolden(goldenDir string, goldenFile string) (string, error) {
// If not enabled, it returns nil.
func NewSpannerClient() *spanner.SpannerClient {
if !EnableSpannerGraph {
log.Println("Spanner graph not enabled.")
slog.Info("Spanner graph not enabled.")
return nil
}
_, filename, _, _ := runtime.Caller(0)
Expand Down