Add tracing from azcore #62
Conversation
|
@jhendrixMSFT what do you think of this? |
go.mod still specifies 1.18, but example_tracing_test.go requires 1.19.
Codecov Report
@@ Coverage Diff @@
## main #62 +/- ##
==========================================
- Coverage 91.05% 90.60% -0.46%
==========================================
Files 6 7 +1
Lines 1174 1256 +82
==========================================
+ Hits 1069 1138 +69
- Misses 73 86 +13
Partials 32 32
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
| "github.com/Azure/azure-sdk-for-go/sdk/azcore/tracing" | ||
| ) | ||
|
|
||
| func (client *Client) startSpan(ctx context.Context, name string) (context.Context, span) { |
There was a problem hiding this comment.
If you want the underlying HTTP trace spans (which you likely do) then you need to use runtime.StartSpan as it adds the underlying tracer to the context for usage in the httpTracePolicy.
There was a problem hiding this comment.
I see. Are third-party libraries intended to use that, though? It's not in an internal package, granted, but seemed more specific to how the runtime works. If I figured wrong, great!
There was a problem hiding this comment.
Good question. Since this is not an official SDK then you don't have to use runtime.StartSpan. And since this SDK isn't making its own HTTP calls but calling into other SDKs then perhaps what you have is sufficient.
| ctx, span := client.startSpan(ctx, "Encrypt") | ||
| defer func() { span.End(err) }() | ||
|
|
||
| client.cache(ctx) | ||
|
|
||
| var encrypter alg.Encrypter | ||
| if alg.As(client.localClient, &encrypter) { | ||
| result, err := encrypter.Encrypt(algorithm, plaintext) |
There was a problem hiding this comment.
err is being shadowed here so span.End() isn't going to report it (I believe there are other instances of this).
There was a problem hiding this comment.
Not if it was already defined: https://go.dev/play/p/A9Kw41FwWXX
I believe this only works because it was not created in a block expression. I remember coming across this in a doc long ago, but having trouble finding it. Still, if you think it's better to declare result explicit then use = to assign both, I could switch to that.
There was a problem hiding this comment.
Try this. It fails to compile because err is declared but not used.
There was a problem hiding this comment.
I see. So scope matters, and a block constitutes a new scope. I've usually only used that in the same scope. This isn't well-documented, it seems. 😔
| var err error | ||
| ctx, span := client.startSpan(ctx, "cache") | ||
| defer func() { span.End(err) }() | ||
|
|
||
| response, err := client.remoteClient.GetKey(ctx, client.keyName, client.keyVersion, nil) |
There was a problem hiding this comment.
Note that this will include a child span of the SDK call as we don't have suppression implemented yet (see the guidelines).
There was a problem hiding this comment.
That's actually what I want - that this tracing is inclusive of any the azkeys module traces. Or did I misunderstand you?
There was a problem hiding this comment.
Let me clarify. In this case, while the span for GetKey will be omitted, any inner spans it would create (e.g. the HTTP span) will show up as a child span of cache.
I have a fix to suppress the nested API call spans which describes this better here.
There was a problem hiding this comment.
I would actually want the GetKey span, though. Would that be solved by me using runtime.StartSpan as you suggested above?
There was a problem hiding this comment.
If you want the GetKey span, then you shouldn't use runtime.StartSpan as it will automatically suppress it (once my fix goes in).
There was a problem hiding this comment.
That seems odd. Why would I want to suppress the important bits? The HTTP request and, more importantly, the response are always good to trace. We don't do it that way in .NET, IIRC.
There was a problem hiding this comment.
An API call creates two spans; a span with the API name (e.g. "GetKey") and a child span with the HTTP request/response. The only span that gets suppressed is the "outer" span with the API name. So, in your trace, you'd see a span named cache with a child span that contains the HTTP request/response, not three spans e.g. cache->GetKey->HTTP request/response.
There was a problem hiding this comment.
Doesn't that presume that an intermediate API call like GetKey does nothing else of value? In .NET, we trace the entire call chain - even simple forward-only helpers.
There was a problem hiding this comment.
That seems to be in contradiction to the design guidelines.
✅ DO When client method creates a new span and internally calls into other public client methods of the same or different Azure SDK, spans created for inner client methods MUST be suppressed, their attributes and events ignored. Nested spans created for REST calls MUST be the children of the outer client call span. Suppression is generally done by Azure Core.
There was a problem hiding this comment.
@annelo-msft or @JoshLove-msft do I remember wrong, or do we not suppress intermediate API calls between convenience methods and actual REST calls? I recall, at least in Key Vault, seeing them both.
No description provided.