Skip to content

Commit

Permalink
move datanode to internal/node (#82)
Browse files Browse the repository at this point in the history
Clues has grown organically into a split between the api that places
data in the right spot (clues) and the structure which contains the data
(nodes). As we move more behavior into subpackages, and as we look
forward to future subpackage additions, it makes the most sense to move
the node systems into an internal package so that they can be
generically utilized across all clues subpackages without issues like
import cycles.

This movement is 80% copy/paste, 19% renaming and reorganization, and 1%
interface/import adaptation. No logical changes were intended.

- copy/paste: datanode.go has moved into /internal/node/node.go. otel.go
has moved into /internal/node/otel.go.
- reorganization: datanode.go has been broken into multiple files.
Discrete concerns (comments, agents, otel) all exist as various files
within /node. Generic and basic functionality stays in /node/node.go.
- adaptation: OTELConfig now has a user-facing config builder in
/clues/otel.go, while the core otel config struct still lives in
/internal/node/otel.go.
  • Loading branch information
ryanfkeepers authored Dec 3, 2024
1 parent f551729 commit 30e3f6a
Show file tree
Hide file tree
Showing 26 changed files with 2,844 additions and 2,832 deletions.
31 changes: 15 additions & 16 deletions best_practices.md
Original file line number Diff line number Diff line change
Expand Up @@ -5,15 +5,15 @@ A guide for getting the most out of Clues.
## CTX

The clues package can leverage golang's `context.Context` to pack in
metadata that you can later retrieve for logging or observation. These
metadata that you can later retrieve for logging or observation. These
additions form a tree, so you're always safe to extend or replace existing
state.

### Tracing

Calling `AddTrace` or `AddTraceName` will append to an internal property
with the key `clues_trace`. Clues traces form a slice of comma delimited
hashes (or names, when using TraceName). These can be useful for filtering
with the key `clues_trace`. Clues traces form a slice of comma delimited
hashes (or names, when using TraceName). These can be useful for filtering
logs to certain process branches.

clues.Add() always automatically appends to the trace, so you don't need
Expand Down Expand Up @@ -57,16 +57,15 @@ for _, user := range users {

Errors are the bottom-to-top metadata tracing counterpart to contexts.
At minimum, they replicate the creation, wrapping, and stacking of
errors. You can also label clues errors for broad categorization and
errors. You can also label clues errors for broad categorization and
add key:value metadata sets, including the full set of clues embedded
in a ctx value.


### Always Stack

A single-error Stack ensures clues will append a stacktrace reference
for that point of return. New(), Wrap() and Stack()ing multiple errors
all do this same process. This tip is specific for cases where you'd
for that point of return. New(), Wrap() and Stack()ing multiple errors
all do this same process. This tip is specific for cases where you'd
normally `return err`.

```go
Expand Down Expand Up @@ -122,8 +121,8 @@ if err != nil {
### Log the Core

The clues within errors aren't going to show up in a log message
or test output naturally. The formatted value will only include
the message. You can easily extract the full details using ToCore().
or test output naturally. The formatted value will only include
the message. You can easily extract the full details using ToCore().

```go
// in logs:
Expand All @@ -135,7 +134,7 @@ assert.NoError(t, err, clues.ToCore(err))
### Labels Are Not Sentinels

Clues errors already support Is and As, which means you can use them
for errors.Is and errors.As calls. This means you shouldn't use
for errors.Is and errors.As calls. This means you shouldn't use
labels for the same purpose.

```go
Expand All @@ -146,26 +145,26 @@ clues.Stack(errByRespCode(resp.StatusCode), err)
```

The best usage of labels is when you want to add _identifiable_ metadata
to an error. This is great for conditions where multiple different
errors can be flagged to get some specific handling. This allows you
to an error. This is great for conditions where multiple different
errors can be flagged to get some specific handling. This allows you
to identify the condition without altering the error itself.

```go
// example 1:
// example 1:
// doesn't matter what error took place, we want to end in a
// certain process state as a categorical result
for err := range processCh {
if clues.HasLabel(err, mustFailBackup) {
// set the backup state to 'failed'
// set the backup state to 'failed'
}
}

// example 2:
// we can categorically ignore errors based on configuration.
// we can categorically ignore errors based on configuration.
for _, err := range processFailures {
for _, cat := range config.ignoreErrorCategories {
if clues.HasLabel(err, cat) {
processFailures.IgnoreError(err)
processFailures.IgnoreError(err)
}
}
}
Expand Down
10 changes: 6 additions & 4 deletions clog/builder.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@ import (
"reflect"

"github.com/alcionai/clues"
"github.com/alcionai/clues/cluerr"
"github.com/alcionai/clues/internal/node"
"github.com/alcionai/clues/internal/stringify"
otellog "go.opentelemetry.io/otel/log"
"go.uber.org/zap"
Expand Down Expand Up @@ -67,11 +69,11 @@ func (b builder) log(l logLevel, msg string) {

if b.err != nil {
// error values should override context values.
maps.Copy(cv, clues.InErr(b.err).Map())
maps.Copy(cv, cluerr.CluesIn(b.err).Map())

// attach the error and its labels
cv["error"] = b.err
cv["error_labels"] = clues.Labels(b.err)
cv["error_labels"] = cluerr.Labels(b.err)
}

// finally, make sure we attach the labels and comments
Expand All @@ -86,15 +88,15 @@ func (b builder) log(l logLevel, msg string) {
for k, v := range cv {
zsl = zsl.With(k, v)

attr := clues.NewAttribute(k, v)
attr := node.NewAttribute(k, v)
record.AddAttributes(attr.KV())
}

// plus any values added using builder.With()
for k, v := range b.with {
zsl.With(k, v)

attr := clues.NewAttribute(stringify.Fmt(k)[0], v)
attr := node.NewAttribute(stringify.Fmt(k)[0], v)
record.AddAttributes(attr.KV())
}

Expand Down
26 changes: 13 additions & 13 deletions clog/builder_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import (
"context"
"testing"

"github.com/alcionai/clues"
"github.com/alcionai/clues/cluerr"
"github.com/stretchr/testify/assert"
)

Expand Down Expand Up @@ -53,17 +53,17 @@ func TestBuilder(t *testing.T) {

bld.SkipCaller(1)

// ensure no collision between separate builders
// using the same ctx.
err := clues.New("an error").
With("fnords", "i have seen them").
Label("errLabel")
// ensure no collision between separate builders
// using the same ctx.
err := cluerr.New("an error").
With("fnords", "i have seen them").
Label("errLabel")

other := CtxErr(ctx, err)
assert.Empty(t, other.with)
assert.Empty(t, other.labels)
assert.Empty(t, other.comments)
assert.ErrorIs(t, other.err, err, clues.ToCore(err))
other := CtxErr(ctx, err)
assert.Empty(t, other.with)
assert.Empty(t, other.labels)
assert.Empty(t, other.comments)
assert.ErrorIs(t, other.err, err, cluerr.ToCore(err))

other.With("foo", "smarf")
assert.Contains(t, other.with, "foo")
Expand Down Expand Up @@ -126,8 +126,8 @@ func runErrorLogs(

func TestGetValue(t *testing.T) {
var (
p1 int = 1
ps string = "ptr"
p1 = 1
ps = "ptr"
pn any
)

Expand Down
21 changes: 21 additions & 0 deletions clog/logger.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package clog

import (
"context"
"errors"
"os"
"sync"
"time"
Expand Down Expand Up @@ -223,6 +224,26 @@ func CtxErr(ctx context.Context, err error) *builder {
return nb
}

// Singleton is a shorthand for .Ctx(context.Background()). IE: it'll use the singleton
// logger directly; building one if necessary. You should avoid this and use .Ctx or
// .CtxErr if possible. Likelihood is that you're somewhere deep in a func chain that
// doesn't accept a ctx, and you still want to add a quick log; maybe for debugging purposes.
//
// That's fine! Everything should work great.
//
// Unless you call this before initialization. Then it'll panic. We do want you to init
// the logger first, else you'll potentially lose these logs due different buffers.
func Singleton() *builder {
if cloggerton == nil {
panic(errors.New("clog singleton requires prior initialization"))
}

return &builder{
ctx: context.Background(),
zsl: cloggerton.zsl,
}
}

// Flush writes out all buffered logs.
// Probably good to do before shutting down whatever instance
// had initialized the singleton.
Expand Down
8 changes: 3 additions & 5 deletions clog/settings.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,16 +6,14 @@ import (

"golang.org/x/exp/slices"

"github.com/alcionai/clues"
"github.com/alcionai/clues/cecrets"
"github.com/alcionai/clues/cluerr"
)

// ---------------------------------------------------
// consts
// ---------------------------------------------------

const clogLogFileEnv = "CLOG_LOG_FILE"

type logLevel string

const (
Expand Down Expand Up @@ -84,14 +82,14 @@ func (s Settings) LogToStdOut() Settings {
// LogToFile defines a system file to write all logs onto.
func (s Settings) LogToFile(pathToFile string) (Settings, error) {
if len(pathToFile) == 0 {
return s, clues.New("missing filepath for logging")
return s, cluerr.New("missing filepath for logging")
}

logdir := filepath.Dir(pathToFile)

err := os.MkdirAll(logdir, 0o755)
if err != nil {
return s, clues.Wrap(err, "ensuring log file dir exists").
return s, cluerr.Wrap(err, "ensuring log file dir exists").
With("log_dir", logdir)
}

Expand Down
4 changes: 2 additions & 2 deletions clog/settings_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import (
"path/filepath"
"testing"

"github.com/alcionai/clues"
"github.com/alcionai/clues/cluerr"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)
Expand Down Expand Up @@ -40,7 +40,7 @@ func TestSettings_LogToFile(t *testing.T) {
for _, test := range table {
t.Run(test.name, func(t *testing.T) {
set, err := Settings{}.LogToFile(test.input)
test.expectErr(t, err, clues.ToCore(err))
test.expectErr(t, err, cluerr.ToCore(err))
assert.Equal(t, test.expectOverride, set.fileOverride)
})
}
Expand Down
Loading

0 comments on commit 30e3f6a

Please sign in to comment.