dd-trace-rb icon indicating copy to clipboard operation
dd-trace-rb copied to clipboard

Avoid diagnostics on rails console

Open sj26 opened this issue 1 year ago • 6 comments

Currently I get a huge DATADOG CONFIGURATION payload output when starting a rails console.

The Rails::Console constant is only loaded when the rails console command is run:

https://github.com/rails/rails/blob/main/railties/lib/rails/commands/console/console_command.rb

It's a semi-conventional way to detect if we're running in a rails console. This PR uses its presence as a signal that we're in a repl, and so avoiding printing the diagnostic information by default.

sj26 avatar Aug 25 '23 04:08 sj26

👋 @sj26, this is a good call!

We actually introduced specific code recently to detect just this kind of scenario, as we needed this detection logic elsewhere in the code: https://github.com/DataDog/dd-trace-rb/blob/65c019dbad0969618b06fc370eb154d27a18c82d/lib/datadog/core/environment/execution.rb#L9-L12

This just wasn't propagated to the EnvironmentLogger yet, given it was undergoing a major refactoring when the above changes were introduced.

marcotc avatar Aug 25 '23 22:08 marcotc

Oh, neat!

I don't think the code you've linked would catch my particular case — a rails console in production. But I could make the change to that core class, if you prefer, and maybe point this environment logger at that core class?

sj26 avatar Aug 25 '23 23:08 sj26

Hm, looking more closely I don't think Datadog::Core::Environment.development? encapsulates what this EnvironmentLogger class wants to know. It's probably closer to interactive?

Which makes me think a more reliable test for what this is trying to do might be "does this process have a tty on stdin", i.e. $stdin.isatty

sj26 avatar Aug 25 '23 23:08 sj26

i.e. I wonder if this would be good:

https://github.com/DataDog/dd-trace-rb/compare/master...sj26:dd-trace-rb:interactive-environment

sj26 avatar Aug 26 '23 00:08 sj26

@sj26, your proposed changes are very clean :) https://github.com/DataDog/dd-trace-rb/compare/master...sj26:dd-trace-rb:interactive-environment

Hm, looking more closely I don't think Datadog::Core::Environment.development? encapsulates what this EnvironmentLogger class wants to know. It's probably closer to interactive?

We wan the environment logger to output information when: It won't bother developers.

So it applies to local REPLs, as well as local test runs or rake tests.

In Rails console in a production environment is probably a place where we don't want it because it's verbose. But it could be helpful if you are having issues with Datadog and is trying to debug something related to it in a Rails console in production. That being said, Rails production consoles are used much more frequently to do things other than debugging Datadog observability, so I disabling seems reasonable.

Do you agree with my description above, @sj26? I want to make sure this would make sense for your use case.

marcotc avatar Aug 28 '23 21:08 marcotc

Yes, that sounds right to me :+1:

The $stdin.tty? thing works for most things you've listed, like rake tasks etc too.

Feel free to use that branch if you like? Or I can pull that in here? Not sure how to do the testing piece though.

sj26 avatar Aug 28 '23 22:08 sj26