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

Adjusted logs to be consistent across the code base #237

Merged
merged 2 commits into from
Feb 28, 2019
Merged

Adjusted logs to be consistent across the code base #237

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
78e387e
Adjusted logs to be consistent across the code base
jpkrohling Feb 27, 2019
adba5c3
Use instance-level logger
jpkrohling Feb 28, 2019
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
15 changes: 7 additions & 8 deletions pkg/cmd/start/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,15 +79,16 @@ func start(cmd *cobra.Command, args []string) {
}

log.WithFields(log.Fields{
"os": runtime.GOOS,
"arch": runtime.GOARCH,
"version": runtime.Version(),
"operator-sdk": version.Get().OperatorSdk,
"os": runtime.GOOS,
"arch": runtime.GOARCH,
"version": runtime.Version(),
"operator-sdk": version.Get().OperatorSdk,
"jaeger-operator": version.Get().Operator,
}).Info("Versions")

namespace, err := k8sutil.GetWatchNamespace()
if err != nil {
log.Fatalf("failed to get watch namespace: %v", err)
log.WithError(err).Fatal("failed to get watch namespace")
}

// Get a config to talk to the apiserver
Expand All @@ -106,7 +107,7 @@ func start(cmd *cobra.Command, args []string) {
log.Debug("Attempting to auto-detect the platform")
os, err := detectOpenShift(mgr.GetConfig())
if err != nil {
log.WithError(err).Info("failed to auto-detect the platform, falling back to 'kubernetes'")
log.WithError(err).Info("Failed to auto-detect the platform, falling back to 'kubernetes'")
}

if os {
Expand All @@ -130,8 +131,6 @@ func start(cmd *cobra.Command, args []string) {
log.Fatal(err)
}

log.Print("Starting the Cmd.")

// Start the Cmd
log.Fatal(mgr.Start(signals.SetupSignalHandler()))
}
Expand Down
7 changes: 5 additions & 2 deletions pkg/config/sampling/sampling.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ package sampling
import (
"fmt"

"github.com/sirupsen/logrus"
log "github.com/sirupsen/logrus"
"k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"

Expand Down Expand Up @@ -40,7 +40,10 @@ func (u *Config) Get() *v1.ConfigMap {
return nil
}

logrus.WithField("instance", u.jaeger.Name).Debug("Assembling the Sampling configmap")
log.WithFields(log.Fields{
Copy link
Member

Choose a reason for hiding this comment

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

I know that you have turned down my opinions but I find the current logging rather ad hoc. I am pretty suer sooner or later somebody will log something without proper fields.

I like static access to logger but in this case I would rather pass it explicitly or use MDC to for consistent logging and avoid repetition every time you access the logger. Another approach is to have a factory method creteLogger(j v1alpha1.Jaeger, tuples...) logger in utils package.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

somebody will log something without proper fields

This happens already, as this PR shows: quite a few places had to be fixed to include the instance and/or namespace.

I'm not against using a base logger for the request-scoped code, but I don't quite like the idea of changing method signatures to include a logger which can be derived from the other parameters (log.WithFields("name": jaeger.Name, "namespace":jaeger.Namespace))

A helper like you proposed would certainly be something to consider, though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a new commit in the direction of your suggestion. How does it look now?

Copy link
Member

Choose a reason for hiding this comment

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

it looks much better 👍

"namespace": u.jaeger.Namespace,
"instance": u.jaeger.Name,
}).Debug("Assembling the Sampling configmap")
trueVar := true

data := map[string]string{
Expand Down
7 changes: 5 additions & 2 deletions pkg/config/ui/ui.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ package configmap
import (
"fmt"

"github.com/sirupsen/logrus"
log "github.com/sirupsen/logrus"
"k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"

Expand Down Expand Up @@ -32,7 +32,10 @@ func (u *UIConfig) Get() *v1.ConfigMap {
return nil
}

logrus.Debug("Assembling the UI configmap")
log.WithFields(log.Fields{
"namespace": u.jaeger.Namespace,
"instance": u.jaeger.Name,
}).Debug("Assembling the UI configmap")
trueVar := true
data := map[string]string{
"ui": string(json),
Expand Down
2 changes: 2 additions & 0 deletions pkg/controller/jaeger/jaeger_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"context"
"fmt"
"strings"
"time"

log "github.com/sirupsen/logrus"
"github.com/spf13/viper"
Expand Down Expand Up @@ -68,6 +69,7 @@ func (r *ReconcileJaeger) Reconcile(request reconcile.Request) (reconcile.Result
logFields := log.WithFields(log.Fields{
"namespace": request.Namespace,
"instance": request.Name,
"execution": time.Now().UTC(),
})

logFields.Debug("Reconciling Jaeger")
Expand Down
6 changes: 5 additions & 1 deletion pkg/deployment/agent.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,11 @@ func NewAgent(jaeger *v1alpha1.Jaeger) *Agent {
// Get returns a Agent pod
func (a *Agent) Get() *appsv1.DaemonSet {
if !strings.EqualFold(a.jaeger.Spec.Agent.Strategy, "daemonset") {
log.WithFields(log.Fields{"instance": a.jaeger.Name, "strategy": a.jaeger.Spec.Agent.Strategy}).Debug("skipping agent daemonset")
log.WithFields(log.Fields{
"instance": a.jaeger.Name,
"namespace": a.jaeger.Namespace,
"strategy": a.jaeger.Spec.Agent.Strategy,
}).Debug("skipping agent daemonset")
return nil
}

Expand Down
7 changes: 5 additions & 2 deletions pkg/deployment/all-in-one.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ package deployment
import (
"fmt"

"github.com/sirupsen/logrus"
log "github.com/sirupsen/logrus"
"github.com/spf13/viper"
appsv1 "k8s.io/api/apps/v1"
"k8s.io/api/core/v1"
Expand Down Expand Up @@ -35,7 +35,10 @@ func NewAllInOne(jaeger *v1alpha1.Jaeger) *AllInOne {

// Get returns a pod for the current all-in-one configuration
func (a *AllInOne) Get() *appsv1.Deployment {
logrus.Debug("Assembling an all-in-one deployment")
log.WithFields(log.Fields{
"instance": a.jaeger.Name,
"namespace": a.jaeger.Namespace,
}).Debug("Assembling an all-in-one deployment")
labels := a.labels()
trueVar := true

Expand Down
5 changes: 4 additions & 1 deletion pkg/deployment/collector.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,10 @@ func NewCollector(jaeger *v1alpha1.Jaeger) *Collector {

// Get returns a collector pod
func (c *Collector) Get() *appsv1.Deployment {
log.WithField("instance", c.jaeger.Name).Debug("assembling a collector deployment")
log.WithFields(log.Fields{
"instance": c.jaeger.Name,
"namespace": c.jaeger.Namespace,
}).Debug("assembling a collector deployment")

labels := c.labels()
trueVar := true
Expand Down
7 changes: 5 additions & 2 deletions pkg/deployment/ingester.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import (
"fmt"
"strings"

"github.com/sirupsen/logrus"
log "github.com/sirupsen/logrus"
"github.com/spf13/viper"
appsv1 "k8s.io/api/apps/v1"
"k8s.io/api/core/v1"
Expand Down Expand Up @@ -41,7 +41,10 @@ func (i *Ingester) Get() *appsv1.Deployment {
return nil
}

logrus.Debugf("Assembling a ingester deployment for %v", i.jaeger)
log.WithFields(log.Fields{
"instance": i.jaeger.Name,
"namespace": i.jaeger.Namespace,
}).Debug("Assembling an ingester deployment")

labels := i.labels()
trueVar := true
Expand Down
7 changes: 5 additions & 2 deletions pkg/deployment/query.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ package deployment
import (
"fmt"

"github.com/sirupsen/logrus"
log "github.com/sirupsen/logrus"
"github.com/spf13/viper"
appsv1 "k8s.io/api/apps/v1"
"k8s.io/api/core/v1"
Expand Down Expand Up @@ -38,7 +38,10 @@ func NewQuery(jaeger *v1alpha1.Jaeger) *Query {

// Get returns a deployment specification for the current instance
func (q *Query) Get() *appsv1.Deployment {
logrus.Debug("Assembling a query deployment")
log.WithFields(log.Fields{
"instance": q.jaeger.Name,
"namespace": q.jaeger.Namespace,
}).Debug("Assembling a query deployment")
labels := q.labels()
trueVar := true
replicas := int32(q.jaeger.Spec.Query.Size)
Expand Down
14 changes: 11 additions & 3 deletions pkg/inject/sidecar.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,20 +26,28 @@ const (
// Sidecar adds a new container to the deployment, connecting to the given jaeger instance
func Sidecar(dep *appsv1.Deployment, jaeger *v1alpha1.Jaeger) {
deployment.NewAgent(jaeger) // we need some initialization from that, but we don't actually need the agent's instance here
logFields := log.WithFields(log.Fields{
"instance": jaeger.Name,
"namespace": jaeger.Namespace,
"deployment": dep.Name,
})

if jaeger == nil || dep.Annotations[Annotation] != jaeger.Name {
log.WithField("deployment", dep.Name).Debug("skipping sidecar injection")
logFields.Debug("skipping sidecar injection")
} else {
decorate(dep)
log.WithField("deployment", dep.Name).Debug("injecting sidecar")
logFields.Debug("injecting sidecar")
dep.Spec.Template.Spec.Containers = append(dep.Spec.Template.Spec.Containers, container(jaeger))
}
}

// Needed determines whether a pod needs to get a sidecar injected or not
func Needed(dep *appsv1.Deployment) bool {
if dep.Annotations[Annotation] == "" {
log.WithField("deployment", dep.Name).Debug("annotation not present, not injecting")
log.WithFields(log.Fields{
"namespace": dep.Namespace,
"deployment": dep.Name,
}).Debug("annotation not present, not injecting")
return false
}

Expand Down
12 changes: 8 additions & 4 deletions pkg/storage/cassandra_dependencies.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ package storage
import (
"fmt"

"github.com/sirupsen/logrus"
log "github.com/sirupsen/logrus"
"github.com/spf13/viper"
batchv1 "k8s.io/api/batch/v1"
"k8s.io/api/core/v1"
Expand All @@ -14,6 +14,10 @@ import (

func cassandraDeps(jaeger *v1alpha1.Jaeger) []batchv1.Job {
trueVar := true
logFields := log.WithFields(log.Fields{
"instance": jaeger.Name,
"namespace": jaeger.Namespace,
})

// TODO should be moved to normalize
if jaeger.Spec.Storage.CassandraCreateSchema.Enabled == nil {
Expand All @@ -27,12 +31,12 @@ func cassandraDeps(jaeger *v1alpha1.Jaeger) []batchv1.Job {

if jaeger.Spec.Storage.CassandraCreateSchema.Datacenter == "" {
// the default in the create-schema is "dc1", but the default in Jaeger is "test"! We align with Jaeger here
logrus.WithField("instance", jaeger.Name).Info("Datacenter not specified. Using 'test' for the cassandra-create-schema job.")
logFields.Info("Datacenter not specified. Using 'test' for the cassandra-create-schema job.")
jaeger.Spec.Storage.CassandraCreateSchema.Datacenter = "test"
}

if jaeger.Spec.Storage.CassandraCreateSchema.Mode == "" {
logrus.WithField("instance", jaeger.Name).Info("Mode not specified. Using 'prod' for the cassandra-create-schema job.")
logFields.Info("Mode not specified. Using 'prod' for the cassandra-create-schema job.")
jaeger.Spec.Storage.CassandraCreateSchema.Mode = "prod"
}

Expand All @@ -42,7 +46,7 @@ func cassandraDeps(jaeger *v1alpha1.Jaeger) []batchv1.Job {

host := jaeger.Spec.Storage.Options.Map()["cassandra.servers"]
if host == "" {
logrus.WithField("instance", jaeger.Name).Info("Cassandra hostname not specified. Using 'cassandra' for the cassandra-create-schema job.")
logFields.Info("Cassandra hostname not specified. Using 'cassandra' for the cassandra-create-schema job.")
host = "cassandra" // this is the default in the image
}

Expand Down
4 changes: 2 additions & 2 deletions pkg/storage/elasticsearch_secrets.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import (
"path/filepath"

"github.com/operator-framework/operator-sdk/pkg/k8sutil"
"github.com/sirupsen/logrus"
log "github.com/sirupsen/logrus"
"k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"

Expand Down Expand Up @@ -99,7 +99,7 @@ func createESCerts(script string) error {
"NAMESPACE="+namespace,
)
if out, err := cmd.CombinedOutput(); err != nil {
logrus.WithFields(logrus.Fields{
log.WithFields(log.Fields{
"script": script,
"out": string(out)}).
Error("Failed to create certificates")
Expand Down
12 changes: 8 additions & 4 deletions pkg/strategy/all-in-one.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ package strategy
import (
"strings"

"github.com/sirupsen/logrus"
log "github.com/sirupsen/logrus"
"github.com/spf13/viper"
appsv1 "k8s.io/api/apps/v1"

Expand All @@ -21,8 +21,12 @@ import (

func newAllInOneStrategy(jaeger *v1alpha1.Jaeger) S {
c := S{typ: AllInOne}
logFields := log.WithFields(log.Fields{
"instance": jaeger.Name,
"namespace": jaeger.Namespace,
})

logrus.Debugf("Creating all-in-one for '%v'", jaeger.Name)
logFields.Debug("Creating all-in-one deployment")

dep := deployment.NewAllInOne(jaeger)

Expand Down Expand Up @@ -69,15 +73,15 @@ func newAllInOneStrategy(jaeger *v1alpha1.Jaeger) S {
if cronjob.SupportedStorage(jaeger.Spec.Storage.Type) {
c.cronJobs = append(c.cronJobs, *cronjob.CreateSparkDependencies(jaeger))
} else {
logrus.WithField("type", jaeger.Spec.Storage.Type).Warn("Skipping spark dependencies job due to unsupported storage.")
logFields.WithField("type", jaeger.Spec.Storage.Type).Warn("Skipping spark dependencies job due to unsupported storage.")
}
}

if isBoolTrue(jaeger.Spec.Storage.EsIndexCleaner.Enabled) {
if strings.EqualFold(jaeger.Spec.Storage.Type, "elasticsearch") {
c.cronJobs = append(c.cronJobs, *cronjob.CreateEsIndexCleaner(jaeger))
} else {
logrus.WithField("type", jaeger.Spec.Storage.Type).Warn("Skipping Elasticsearch index cleaner job due to unsupported storage.")
logFields.WithField("type", jaeger.Spec.Storage.Type).Warn("Skipping Elasticsearch index cleaner job due to unsupported storage.")
}
}

Expand Down
36 changes: 20 additions & 16 deletions pkg/strategy/controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import (
"fmt"
"strings"

"github.com/sirupsen/logrus"
log "github.com/sirupsen/logrus"
"github.com/spf13/viper"

"github.com/jaegertracing/jaeger-operator/pkg/apis/io/v1alpha1"
Expand All @@ -15,14 +15,19 @@ import (

// For returns the appropriate Strategy for the given Jaeger instance
func For(ctx context.Context, jaeger *v1alpha1.Jaeger) S {
logFields := log.WithFields(log.Fields{
"instance": jaeger.Name,
"namespace": jaeger.Namespace,
})

if strings.EqualFold(jaeger.Spec.Strategy, "all-in-one") {
logrus.Warnf("Strategy 'all-in-one' is no longer supported, please use 'allInOne'")
logFields.Warn("Strategy 'all-in-one' is no longer supported, please use 'allInOne'")
jaeger.Spec.Strategy = "allInOne"
}

normalize(jaeger)

logrus.Debugf("Jaeger strategy: %s", jaeger.Spec.Strategy)
logFields.WithField("strategy", jaeger.Spec.Strategy).Debug("Strategy chosen")
if strings.EqualFold(jaeger.Spec.Strategy, "allinone") {
return newAllInOneStrategy(jaeger)
}
Expand All @@ -39,23 +44,26 @@ func For(ctx context.Context, jaeger *v1alpha1.Jaeger) S {
func normalize(jaeger *v1alpha1.Jaeger) {
// we need a name!
if jaeger.Name == "" {
logrus.Infof("This Jaeger instance was created without a name. Setting it to 'my-jaeger'")
jaeger.Name = "my-jaeger"
log.WithField("instance", jaeger.Name).Info("This Jaeger instance was created without a name. Applying a default name.")
}

logFields := log.WithFields(log.Fields{
"instance": jaeger.Name,
"namespace": jaeger.Namespace,
})

// normalize the storage type
if jaeger.Spec.Storage.Type == "" {
logrus.Infof("Storage type wasn't provided for the Jaeger instance '%v'. Falling back to 'memory'", jaeger.Name)
logFields.Info("Storage type not provided. Falling back to 'memory'")
jaeger.Spec.Storage.Type = "memory"
}

if unknownStorage(jaeger.Spec.Storage.Type) {
logrus.Infof(
"The provided storage type for the Jaeger instance '%v' is unknown ('%v'). Falling back to 'memory'. Known options: %v",
jaeger.Name,
jaeger.Spec.Storage.Type,
storage.ValidTypes(),
)
logFields.WithFields(log.Fields{
"storage": jaeger.Spec.Storage.Type,
"known-options": storage.ValidTypes(),
}).Info("The provided storage type is unknown. Falling back to 'memory'")
jaeger.Spec.Storage.Type = "memory"
}

Expand All @@ -67,11 +75,7 @@ func normalize(jaeger *v1alpha1.Jaeger) {
// check for incompatible options
// if the storage is `memory`, then the only possible strategy is `all-in-one`
if strings.EqualFold(jaeger.Spec.Storage.Type, "memory") && !strings.EqualFold(jaeger.Spec.Strategy, "allinone") {
logrus.Warnf(
"No suitable storage was provided for the Jaeger instance '%v'. Falling back to all-in-one. Storage type: '%v'",
jaeger.Name,
jaeger.Spec.Storage.Type,
)
logFields.WithField("storage", jaeger.Spec.Storage.Type).Warn("No suitable storage provided. Falling back to all-in-one")
jaeger.Spec.Strategy = "allInOne"
}

Expand Down
Loading