ddtrace/tracer: add NoDebugStack finishing option #355
Conversation
|
The purpose of opening an issue before a PR is to first discuss a solution before writing code. Let’s figure out how we’ll implement this and then come back to writing code. I hope that’s ok with you. |
|
@gbbr why did you close the PR? This way you discourage people fixing issues |
|
Just for you reference I can't reopen this PR |
I apologize for not replying earlier, for some reason I did not get any notification from Github that you have written these messages. My intention is in no way to discourage you or anyone from submitting changes to the repository. I also apologize if my comment was too brief. The reason I have closed it is because of our contribution guidelines (here) where the first paragraph says:
This is normal open-source workflow and is nothing specific to our project or out of the ordinary. Our contribution guidelines file is also standard for Github and a link to it is presented when opening a Pull Request. I am sorry if you were not aware of this. My intention was in no way to make you feel like your work was rejected. I was simply saying that we should go back to discussing it before writing code and follow the same workflow as everyone else. Perhaps closing the PR gave you the wrong impression.
You can if you push your local copy of the branch. I have also restored the remote branch for you, if we will have to reopen it. For future reference, I'd recommend reading Github's "Open Source Guide". It provides a lot of useful information about how to contribute to open source projects. I hope you are still willing to discuss a solution to this in the issue and reach a consensus together. I'd be happy if you'd re-open your PR and make the changes we agree on, after we agree on them on the linked issue. |
|
Re-opening as per discussion in #354 |
ddtrace/tracer/option.go
Outdated
| @@ -161,3 +161,10 @@ func WithError(err error) FinishOption { | |||
| cfg.Error = err | |||
| } | |||
| } | |||
|
|
|||
| // Disable backtrace generation in case of error. | |||
There was a problem hiding this comment.
You'll have to phrase this starting the name of the identifier, otherwise it's not standard and the linter will warn you:
// NoDebugStack prevents any error presented using the WithError finishing option
// from generating a stack trace. This is useful in situations where errors are frequent
// and performance is critical.
|
CI is failing with an unrelated to this PR error: @gbbr do you have a fix for that? |
| finishOptions := []tracer.FinishOption{ | ||
| tracer.WithError(err), | ||
| } | ||
| if noDebugStack { |
gbbr
changed the title
I will resolve this. We should maybe remove support for this library. They are making breaking changes every week. |
gbbr
force-pushed
the
dm/optional-trace-back
branch
from
a8f970f
to
1c136a5
Compare
gbbr
force-pushed
the
dm/optional-trace-back
branch
from
1c136a5
to
50d5b49
Compare
|
Thanks! Will include this in the release this following week! |
|
Thanks @gbbr Indeed I've forgotten to remove the code I've added for |
Adds a new finish option to the tracer which does not cause the generation of a stack trace on errors for performance reasons in some scenarios. Additionally, it adds this as opt-in to the grpc integration. Fixes DataDog#354
Traceback generation in Go is quite costly operation and as errors such as BE is throttled might occur very often the the application end up just burning CPU on generating the backtraces.
On top of that the gRPC backtraces are super long which put pressure on GC and also they are completely useless as they all end up in gRPC's stack not pointing to a caller
Address #354