Add tracing module #335
Add tracing module #335
Conversation
vladbarosan
commented
Oct 16, 2018
- add tracing support. Instrumentation is done through a custom RoundTripper on the http client and through SDK function calls with the goal of correlating sdk calls with the api calls. Also convenience tracing calls are added.
- remove usage of original request's context on LRO polling.
|
Initially wanted to place this inside azure path as it makes more sense, but in that case would be painful to use it for adal. In the end I decided to use it for adal calls also since that would provide value to customers. |
autorest/tracing/tracing.go
Outdated
| func Enable() (err error) { | ||
| Enabled = true | ||
| sampler = nil | ||
| Transport = &ochttp.Transport{Propagation: &tracecontext.HTTPFormat{}} |
There was a problem hiding this comment.
note we updated for the W3C's Trace Context for propagating across servers instead of the B3 default implementation. Since Trace Context is on its way of becoming the standard but still a draft a little torn here. @joshgav regarding the ARM tracing, for immediate practical effects using the Trace Context means having only 1 header with any custom key-value pairs as opposed to some predefined headers for the B3 propagation .
6b3039c to
441dc1c
Compare
441dc1c to
0632909
Compare
d6e3b90 to
979456a
Compare
tracing/tracing.go
Outdated
| } | ||
|
|
||
| // Enable will start instrumentation for metrics and traces. | ||
| func Enable() (err error) { |
There was a problem hiding this comment.
Should there be a matching Disable()?
There was a problem hiding this comment.
not sure why they would want to disable once it started but who knows. good point will see if I can add it
There was a problem hiding this comment.
If adding Disable() can be done in a non-breaking way then we don't have to hold up this PR for it.
There was a problem hiding this comment.
well it could but still investigating if that might cause any issue. im still testing scenarios to see if anything breaks
tracing/tracing.go
Outdated
| Transport = http.DefaultTransport | ||
|
|
||
| // Enabled is the flag for marking if tracing is enabled. | ||
| Enabled = false |
There was a problem hiding this comment.
Do you expect customers to use this value or is it purely for testing purposes? If it's the latter then perhaps we can not export it.
There was a problem hiding this comment.
Or make an enabled() func that predicates on sampler being nil or not.
There was a problem hiding this comment.
well, I was thinking they might use it to check if monitoring is enabled on the sdk. if we want we could leave it unexported and if there is a requirement we can export it.
There was a problem hiding this comment.
I see. It might be better to make it Enabled() then so it's effectively read-only (I could see somebody setting it to false thinking it would turn it off).
0a9ea3d to
9e82245
Compare
0588420 to
4f7ddca
Compare
4f7ddca to
3cdf167
Compare