secrets: migrate secrets to utilize opentelemetry #3547
Conversation
secrets/secrets.go
Outdated
| otel.Handle(err) | ||
| } | ||
|
|
||
| completedCallsCounter, err = meter.Int64Counter( |
There was a problem hiding this comment.
Why do we need this? Doesn't the latency histogram implicitly have a call count?
There was a problem hiding this comment.
@vangent I tried to maintain existing functionality here, I have no strong opinion on it and can get rid of it if its preferred not to exist
.
There was a problem hiding this comment.
In the existing functionality, I only see one metric, a "latencyMeasure"....?
There was a problem hiding this comment.
So the other "completed_calls" was being added through a view, I have tried to spend sometime to replicate the functionality. To be able to use the latency measure as its being used now.
secrets/secrets.go
Outdated
| metric.WithDescription("Latency distribution of method calls"), | ||
| ) | ||
| if err != nil { | ||
| otel.Handle(err) |
There was a problem hiding this comment.
What does this do, and in what state does it leave the latencyHistogram?
There was a problem hiding this comment.
The otle.Handle(err) is a global handler for errors so for sure there will be an error, however now I see we could endup with a partially configured latencyHistogram. Just looking further into this.
There was a problem hiding this comment.
What does "there will be an error" mean though? Does the application crash? Or does it just log something and return? If the latter, what happens later one when we try to record metrics?
There was a problem hiding this comment.
I have added a panic here, I will need to spend sometime to ensure this error is only related to things like invalid names being used or things which can be caught during development and not something like unable to provision the infrastracture for the meter.
secrets/secrets.go
Outdated
| defer func() { | ||
| // Set status on span before ending | ||
| if err != nil { | ||
| span.SetAttributes(attribute.String("gocdk.status", "13")) // Internal error |
There was a problem hiding this comment.
Where did you get 13? Isn't there a constant for it somewhere?
There was a problem hiding this comment.
This is cleaned up
secrets/secrets.go
Outdated
| defer func() { k.tracer.End(ctx, err) }() | ||
| start := time.Now() | ||
| ctx, span := k.tracer.Start(ctx, "Decrypt") | ||
| // Set span attributes for testing |
There was a problem hiding this comment.
So, it's not great that we went from 2 lines of code to record metrics to about 30. I suspect this will happen in every package. It's repetitive even inside this package (lines 130-160, lines 178-209 are basically the same).
a) Why does OpenTelemetry need so much more? For example, you're already passing the method name to the OpenSpan, why do we need to provide it again as an attribute?
b) Can you add a wrapper Start/End in the otel/ package that could be called here so that secrets/blob/pubsub/etc. only have to call Start and defer End?
There was a problem hiding this comment.
This is fixed now
There was a problem hiding this comment.
@vangent I would like you to check the view in internal/otel/metrics.go you might have a better idea
…/go-cloud into otel-migration-secrets
Micro PR to migrate monolith : #3539
PR affects :
secrets