Remove a stray fmt.Printf, use a logger #14468
Conversation
atoulme
force-pushed
the
fix_ottl_printf
branch
3 times, most recently
from
3befc4b to
4eeb6df
Compare
atoulme
force-pushed
the
fix_ottl_printf
branch
from
4eeb6df to
e357a02
Compare
There was a problem hiding this comment.
I'm home with COVID at the moment and not in a position to do any performance analysis today.
But I'm not very happy about adding a logger parameter everywhere and passing it on every function call. This code is designed to be as fast as possible.
The printf you're complaining about iin #14467 is telling you that you gave it bad data -- the example code in the bug (please link the bug with # before the bug number in the original so that GitHub does the right things with it!) is telling you that attributes["foo"] is not a string but you're comparing it to one.
In the original version of this code, I didn't have the printf at all, and then was asked to include it because we should generate some.
I guess if we think we need it, I would much prefer to move all of the functions here into member functions on the parser. There are also benchmarks in compare_test.go that I'd want to run a before/after comparison with, whichever path we take.
And finally, I don't believe that either transformprocessor or ottl has introduced a logger before. If we do, I don't think I want to depend on zap as the standard logging interface; I would prefer a more generic choice, especially now that there's some consideration going on in the Go community towards a more flexible standard logging model. Given that this is just a single call, I'd suggest we start with the Go standard log interface, or even define a logger interface locally that we can tweak over time.
@TylerHelmuth do you have an opinion on any of this?
|
@kentquirk I agree with everything you said except for not depending on zap. When @evan-bradley introduced the generic logger framework that was definitely the correct solution, but since that decision was made we've pivoted OTTL away from an agnostic solution and steered it towards a collector-specific solution. With the pivot in mind, I think zap makes sense since the collector's logger of choice is zap. The OTTL parser now requires the collector's TelemetrySettings struct, which includes its zap logger. (You we're out for this PR but still willing to discuss if you have thoughts). |
|
I would also say we do want to replace the print statement with the logger usage so it can conform with the collectors logging settings. I agree that I would rather not pass it around. |
|
Ok, thanks. I agree that if the collector has standardized on zap that we can use that (although I wish it had been a more general solution, it's done now). So then, the right answer here is, I think, to move the compare functions into private member functions so that we don't have to add the logger to their signatures. |
Please rest up!
It would be best not to print out anything then.
This is not bad data - attributes["foo"] is a nil value, and this new behavior now makes us print this line for every record, which is a regression of our system. We have customers asking for a fix at once. There is a simpler fix - we delete the fmt.Printf line. |
|
This print statement is serving a useful purpose of letting customers know when there are issues or unexpected outcomes during hot path execution. The issue this print statement is informing users about cannot be determined during collector validation. For this reason we need to continue to inform users of this situation. This print statement, as well as other situations we haven't addressed yet in Context and Functions, should use the collectors logger so that the processor honors the collectors telemetry settings for logging. |
|
Just to summarize what was discussed here and in the issue. We all agree that the log message is valid, but should be replaced with a debug log entry instead. This PR reaches the goal, but passing the logger around as an argument is not ideal solution, so we want to move the functions to member functions on the parser. @TylerHelmuth @kentquirk please correct me if I'm missing anything. My question if we can merge this PR as is and handle the refactoring mentioned above as part of another issue. @TylerHelmuth @kentquirk WDYT? |
|
@dmitryax you've got it all summarized correctly. I'd really rather not fix #14467 via passing the logger around as it is a fix where we are instantly introducing more work for ourselves and potentially degrading performance (we have benchmarks for comparison to test this). If we can't fix the issue using member functions at this time and absolutely need to deal with the spurious logs before the next release I'd rather the print statement be removed entirely and a new, high priority issue opened to address passing logging around the parser. This isn't ideal, but it feels better than adding code we already know we want to remove that could affect performance. @atoulme if you can run the comparison benchmarks in the package on the main branch and on this PR's branch and confirm there is no performance degradation, then I'd be more inclined to handled the issue via passing the logger. |
|
New benchmark: goos: darwin goos: darwin |
Co-authored-by: Dmitrii Anoshin <[email protected]>
TylerHelmuth
added
the
ready to merge
Description:
Remove a stray fmt.Printf, use a logger
Link to tracking Issue:
#14467
Testing:
N/A
Documentation:
N/A