ddtrace/tracer: add interchangeable writer to logTraceWriter #939
Conversation
The TestTracerCleanStop test had LambdaMode added to it, which generates a lot of output for the tests. There is no reason why running the test in LambdaMode should improve this test.
There was a problem hiding this comment.
Should be fine. This was added because the initial implementation of the referenced PR (#859) triggered agent discovery at startup, and that severely stalled this particular test due to the repeated HTTP calls. The whole thing was later placed behind a feature flag, technically removing that issue.
It's fine for now, but when the feature flag will be on by default, the discovery will significantly stall this test, in which case, we might want to consider leaving lambda mode there and instead making sure the output isn't logged.
As a precautionary measure, I think the best improvement would be to do that as of this PR: disable logging by configuring the output of lambda mode to be some /dev/null writer. Otherwise, when the feature will become enabled again, we'll be back to square one...
I will leave it up to you to decide, I'm fine with either outcome.
…g/dd-trace-go into knusbaum/revert-clean-stop-lambda
|
@gbbr Cool, that's good to know. I've put the lambda mode back in that case, but added an unexported function to switch out the |
ddtrace/tracer/writer.go
Outdated
|
|
||
| // setLogWriter sets the io.Writer that any new logTraceWriter will write to and returns a function | ||
| // which will return the io.Writer to its original value. | ||
| func setLogWriter(w io.Writer) func() { |
There was a problem hiding this comment.
I think this func is only needed in tests right? If so, does it make sense to move it into a test file?
There was a problem hiding this comment.
I considered it, but since var logWriter is declared and used here, I thought the function for manipulating it should be co-located so the intention/usage is apparent for whoever next looks at var logWriter.
Let me know if you feel strongly that it should be moved.
There was a problem hiding this comment.
I think it's test code (unused elsewhere), so it belongs in a test. But likewise, if you feel strongly, leave it :)
Co-authored-by: Gabriel Aszalos <[email protected]>
|
@gbbr Fixed. Please check it out. |
gbbr
changed the title
gbbr
changed the title
gbbr
changed the title
The TestTracerCleanStop test had LambdaMode added to it, which generates a
lot of output for the tests.
(See: https://app.circleci.com/pipelines/github/DataDog/dd-trace-go/1442/workflows/9add3fc7-f60c-4d09-b370-1af3aa5a851a/jobs/9866)
There is no reason why running the test in LambdaMode should improve this test.
This change seems to have been made in #859, but it doesn't look related.
(Maybe @gbbr can comment why we want to do this)
If we want to test LambdaMode too, we can duplicate the test for lambda mode,
but we should solve the test output issue.