Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid diagnostics on rails console #3079

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

sj26
Copy link

@sj26 sj26 commented Aug 25, 2023

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.

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.
@sj26 sj26 requested a review from a team August 25, 2023 04:01
@github-actions github-actions bot added the core Involves Datadog core libraries label Aug 25, 2023
@marcotc
Copy link
Member

marcotc commented Aug 25, 2023

👋 @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:

# Is this process running in a development environment?
# This can be used to make decisions about when to enable
# background systems like worker threads or telemetry.
def development?

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

@sj26
Copy link
Author

sj26 commented Aug 25, 2023

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
Copy link
Author

sj26 commented Aug 25, 2023

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
Copy link
Author

sj26 commented Aug 26, 2023

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

master...sj26:dd-trace-rb:interactive-environment

@marcotc
Copy link
Member

marcotc commented Aug 28, 2023

@sj26, your proposed changes are very clean :) 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 marcotc self-assigned this Aug 28, 2023
@sj26
Copy link
Author

sj26 commented Aug 28, 2023

Yes, that sounds right to me 👍

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.

@ivoanjo
Copy link
Member

ivoanjo commented Dec 9, 2024

I'm not quite sure if this is still relevant 👀...?

In particular we've done some work to reduce the default logs (#3448, #3785, etc) in version 2.x of dd-trace-rb.

@marcotc thoughts? @sj26 do you still find it very annoying on the newer gem versions?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Involves Datadog core libraries
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants