Don't trace /healthz #844
Don't trace /healthz #844
Conversation
|
/cc @kelseyhightower |
|
I will update the PR to also add |
plugin/ochttp/client.go
Outdated
| @@ -57,6 +57,9 @@ type Transport struct { | |||
| // RoundTrip implements http.RoundTripper, delegating to Base and recording stats and traces for the request. | |||
| func (t *Transport) RoundTrip(req *http.Request) (*http.Response, error) { | |||
| rt := t.base() | |||
| if req.URL.Path == "/healthz" { | |||
There was a problem hiding this comment.
Thanks for the quick fix @rakyll!
Could we please add a comment for the rationale of this change?
// Disable tracing /healthz since tracing it is costly and
// it bloats Kubernetes traces without adding much value
|
Also needs support for the server. |
|
Needs tests for the client spans. |
Kubernetes users end up having tons of /healthz traces which is costly and no-value. Disable /healthz tracing until we have a better solution.
|
I feel this is rushed in, which might have been necessary but I'd like to revisit with a better solution for the future... this just hard coded a couple of paths to disable tracing. I can see where we'd want it configurable depending on the consumer's ecosystem and maybe make it possible to decide path based sampling strategy instead of only a hard drop all for certain paths. that would allow for more functionality while also providing a good solution for this situation (with these paths using the neverSample strategy) |
|
agree with @basvanbeek, this should have been discussed more broadly probably on the specs repo. what was the justification for rushing it in like this? |
|
Sorry, this was rushed in. I created census-instrumentation/opencensus-specs#151 but haven't proposed anything new such as a blacklisting mechanism. Let's discuss on the specs what we want to do and fix the implementation accordingly. |
|
Agree with Bas, we should have a simple blacklist like OC Ruby (I think) does. For example, it'd be great to not trace requests for Stackdriver Profiler. |
|
@mtwo, can you update the spec issue I have field? Let's keep the conversation over there. |
Kubernetes users end up having tons of /healthz traces
which is costly and no-value.
Disable /healthz tracing until we have a better solution.