Create helpers which integrate with OpenTelemetry for diagnose collection #11454
Conversation
vault/diagnose/output.go
Outdated
| for i := 0; i < depth; i++ { | ||
| sb.WriteRune('\t') | ||
| } | ||
| sb.WriteString(" ") |
There was a problem hiding this comment.
I'm working on visualizing what is going on here.
It looks like general nesting is happening with the \t parts, and then warnings are further nested under the r.Message by writing the empty spaces.
Do we want the first of the warning to have a newline between it and the r.Message? It looks like there will just be spaces, then a warning (on the same line as the message), then more warnings on each line after.
There was a problem hiding this comment.
Possibly, this was quick and dirty. but that does look like a bug as written. Maybe I should throw up a unit test so we can take a look.
| tc = NewTelemetryCollector() | ||
| //so, _ := stdout.NewExporter(stdout.WithPrettyPrint()) | ||
| tp = sdktrace.NewTracerProvider( | ||
| sdktrace.WithSampler(sdktrace.AlwaysSample()), |
There was a problem hiding this comment.
The docs say AlwaysSample returns a Sampler that samples every trace. Be careful about using this sampler in a production application with significant traffic: a new trace will be started and exported for every request. . Would this be a problem?
https://pkg.go.dev/go.opentelemetry.io/otel/[email protected]/trace#Sampler
There was a problem hiding this comment.
We'd only want to use this in Diagnose runs, likely, yes. Not in general.
|
|
||
| // Test creates a new named span, and executes the provided function within it. If the function returns an error, | ||
| // the span is considered to have failed. | ||
| func Test(ctx context.Context, spanName string, function func(context.Context) error, options ...trace.SpanOption) error { |
There was a problem hiding this comment.
This is an awesome function!
So, say I wanted to call Test a function, but I also wanted that function to have record actions. Would I need 2 different spans -- one for Test, and one within the function itself?
There was a problem hiding this comment.
Test introduces a span, so you probably only need the one. You can of course have more spans nested under the Test (started by the function or sub-calls).
|
|
||
| // Warn records a warning on the current span | ||
| func Warn(ctx context.Context, msg string) { | ||
| span := trace.SpanFromContext(ctx) |
There was a problem hiding this comment.
So to use these functions, we need to add a span to our context? Suppose we're in a particular context for a function a, and we want to call Test on a sub function b -- do we create a new context with a new spanName?
There was a problem hiding this comment.
These ones yes. Let me throw up a unit test with a working example which may help.
vault/diagnose/helpers.go
Outdated
| } | ||
|
|
||
| func Action(actionName string) trace.LifeCycleOption { | ||
| return trace.WithAttributes(attribute.String(actionKey, actionName)) |
There was a problem hiding this comment.
I'm not sure how Action ties into a span. Could you add a comment?
There was a problem hiding this comment.
It's actually from an earlier effort, removing.
vault/diagnose/output.go
Outdated
| } | ||
| } | ||
|
|
||
| func (t *TelemetryCollector) OnStart(parent context.Context, s sdktrace.ReadWriteSpan) { |
There was a problem hiding this comment.
What does parent do here? Could you leave a quick comment?
vault/diagnose/output.go
Outdated
|
|
||
| func (t *TelemetryCollector) OnEnd(e sdktrace.ReadOnlySpan) { | ||
| if !e.Parent().HasSpanID() { | ||
| // Deep first walk the span structs to construct the top down tree results we want |
There was a problem hiding this comment.
I'm a bit confused about how this works. Why does this call getOrBuildResult 2 levels deep, as opposed to recursively until p == nil ?
There was a problem hiding this comment.
It doesn't need to since the span map is all spans, so all nodes that are parents will be visited once. Some Results will have already been built when they're visited if they're a parent to an earlier span.
vault/diagnose/output.go
Outdated
| } | ||
| } | ||
|
|
||
| func (t *TelemetryCollector) Shutdown(ctx context.Context) error { |
There was a problem hiding this comment.
Could you leave some quick comments on these stub methods detailing what we'd want them to do?
Co-authored-by: swayne275 <[email protected]>
…lt into diagnose-otel-integration
command/operator_diagnose.go
Outdated
| _, err = server.setupStorage(config) | ||
| return err | ||
| }) | ||
| return nil |
There was a problem hiding this comment.
hmm, it looks like you're returning right after another return?
There was a problem hiding this comment.
this github preview doesn't show enough code to show what i'm pointing out
There was a problem hiding this comment.
Can you take another look? It's tricky because there is an anonymous function there as an argument to diagnose.Test, so you're probably seeing that functions return followed by the other one.
There was a problem hiding this comment.
yeah, this is what i'm seeing. I'm not thinking about the return err bit inside the func(ctx context.Context) error, i'm thinking about the return diagnose.Test... on line 219
thoughts?
There was a problem hiding this comment.
basically i think the return nil is a noop - and i'm guessing the compiler doesn't complain about that if the tests are running?
…ration # Conflicts: # command/operator_diagnose.go # command/operator_diagnose_test.go
…tion (hashicorp#11454) * Create helpers which integrate with OpenTelemetry for diagnose collection * Go mod vendor * Comments * Update vault/diagnose/helpers.go Co-authored-by: swayne275 <[email protected]> * Add unit test/example * tweak output * More comments * add spot check concept * Get unit tests working on Result structs * Fix unit test * Get unit tests working, and make diagnose sessions local rather than global * Comments * Last comments * No need for init * :| * Fix helpers_test Co-authored-by: swayne275 <[email protected]>
…tion (hashicorp#11454) * Create helpers which integrate with OpenTelemetry for diagnose collection * Go mod vendor * Comments * Update vault/diagnose/helpers.go Co-authored-by: swayne275 <[email protected]> * Add unit test/example * tweak output * More comments * add spot check concept * Get unit tests working on Result structs * Fix unit test * Get unit tests working, and make diagnose sessions local rather than global * Comments * Last comments * No need for init * :| * Fix helpers_test Co-authored-by: swayne275 <[email protected]>
No description provided.