Skip to content
This repository was archived by the owner on Aug 23, 2023. It is now read-only.

disable tracing for healthchecks #1054

Merged
merged 1 commit into from
Sep 24, 2018
Merged

Conversation

replay
Copy link
Contributor

@replay replay commented Sep 19, 2018

I'm not sure what the best method to disable tracing on certain routes is. We could also just have a list of urls where tracing should be disabled, then we could compare the url against that list in api/middleware/tracer.go, but I think maintaining such a list is going to be ugly.

We could also just not add the tracer middleware with r.Use() anymore, but add it in those routes that should have tracing enabled separately, but then we wouldn't have the tracer in all the middlewares that get executed before that one because then the tracer would be later in the list of middlewares.

@replay replay requested a review from Dieterbe September 19, 2018 14:30
@replay replay force-pushed the disable_tracing_for_healthchecks branch from cc623b7 to 5c476f6 Compare September 19, 2018 14:32
if noTrace, ok := macCtx.Data["noTrace"]; ok && noTrace.(bool) {
return
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do all the work up to here if we don't need tracing? can't we do this at the start of this function?

Copy link
Contributor Author

@replay replay Sep 20, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in the beginning of this function macCtx.Data["noTrace"] would not have been set yet, because the tracer is at the beginning of the list of handlers before the one that sets noTrace. I think it probably makes sense to leave the tracer at it's position in the middleware chain, so that we have it available in the other middlewares if we want to use it there.

@replay
Copy link
Contributor Author

replay commented Sep 24, 2018

Tested like this:

  • Built MT image from this branch
  • Brought up docker-dev-custom-cfg-kafka
  • Called curl -H 'X-Org-Id: 1' http://localhost:6060/render -H 'Content-Type: application/json' -d '{"target": ["*"]}', received expected response []
  • Checked Jaeger and saw a trace for the above /render call
  • Called curl -H 'X-Org-Id: 1' http://localhost:6060/node, received expected response
  • Checked Jaeger and verified that there is no new trace for the call to /node
  • Called curl -H 'X-Org-Id: 1' http://localhost:6060, received expected response OK
  • Checked Jaeger again and verified that there's no new trace for the above call
  • Called curl -H 'X-Org-Id: 1' http://localhost:6060/cluster, got expected response
  • Checked Jaeger to verify that there is a trace for the /cluster call

Looks good afaict

@Dieterbe
Copy link
Contributor

great. thanks mauro!

@Dieterbe Dieterbe merged commit 5a3b3dc into master Sep 24, 2018
@Dieterbe Dieterbe deleted the disable_tracing_for_healthchecks branch October 29, 2018 09:07
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants