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

Make additional managed + native logging improvements #1025

Merged
merged 5 commits into from
Nov 10, 2020

Conversation

zacharycmontoya
Copy link
Collaborator

Makes planned improvements to the managed and native tracer logging:

  • Both
    • Adds a new DD_TRACE_LOG_DIRECTORY environment variable to take precedence over DD_TRACE_LOG_PATH
  • Native
    • Renames output file from dotnet-profiler.log to dotnet-tracer-native.log
    • Adds Windows/Linux to the native log output
    • Change the default behavior to only print out the Datadog environment variables that are set. When set to debug mode, the native logs will write all Datadog environment variables, including empty ones
  • Managed
    • Adds unit tests for dynamically setting the managed tracer's log-level

One additional fix is made: Do not use the EnvironmentHelpers utility class in the EnvironmentConfigurationSource class. This creates an indirect dependency from the configuration system to the logging system.

@DataDog/apm-dotnet

…ence over the previous logging configuration key DD_TRACE_LOG_PATH and makes it clear that the developer specifies one directory to which all .NET Tracer logs will be written.

This feature is backwards-compatible:
  1. If DD_TRACE_LOG_PATH was not used, the managed and native log files will be written to the same default path.
  2. If DD_TRACE_LOG_PATH was used (and the new DD_TRACE_LOG_DIRECTORY configuration key is not used), then the .NET Tracer will the parent directory of the input will continue to write all log files to the parent directory.
@zacharycmontoya zacharycmontoya added area:tracer The core tracer library (Datadog.Trace, does not include OpenTracing, native code, or integrations) area:tests unit tests, integration tests area:native-library Automatic instrumentation native C++ code (Datadog.Trace.ClrProfiler.Native) labels Nov 10, 2020
@zacharycmontoya zacharycmontoya requested a review from a team as a code owner November 10, 2020 00:21
@zacharycmontoya zacharycmontoya self-assigned this Nov 10, 2020
@tonyredondo
Copy link
Member

Now that I'm reviewing this PR I have a question, on Linux we work in the assumption that we have write access to /var/log/datadog/dotnet/. What happens when we don't?, A critical thing, do we crash?. Second thing, can't we use the ˜ home folder in those cases?

@@ -91,7 +91,10 @@ CorProfiler::Initialize(IUnknown* cor_profiler_info_unknown) {
Info("Environment variables:");

for (auto&& env_var : env_vars_to_display) {
Info(" ", env_var, "=", GetEnvironmentValue(env_var));
WSTRING env_var_value = GetEnvironmentValue(env_var);
if (debug_logging_enabled || !env_var_value.empty()) {
Copy link
Member

@lucaspimentel lucaspimentel Nov 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! This always bothered me, especially as we add more and more settings.

@lucaspimentel
Copy link
Member

lucaspimentel commented Nov 10, 2020

@tonyredondo:

on Linux we work in the assumption that we have write access to /var/log/datadog/dotnet/. What happens when we don't?

We used to have a try/catch (source). We didn't crash, but we didn't write any logs, either. I don't know what happens now since we migrated to spdlog. We should definitely test this.

Second thing, can't we use the ˜ home folder in those cases?

Seems like a reasonable fallback. One concern I have is that users won't think to look there for the logs. We should make sure to update the docs and inform the Support Team about this change. Logs that can't be found are as good as no logs 😉 This will be less of an issue once we are able to include tracer logs in the Agent Flare.

@tonyredondo
Copy link
Member

@tonyredondo:

on Linux we work in the assumption that we have write access to /var/log/datadog/dotnet/. What happens when we don't?

We used to have a try/catch (source). We didn't crash, but we didn't write any logs, either. I don't know what happens now since we migrated to spdlog. We should definitely test this.

Second thing, can't we use the ˜ home folder in those cases?

Seems like a reasonable fallback. One concern I have is that users won't think to look there for the logs. We should make sure to update the docs and inform the Support Team about this change. Logs that can't be found are as good as no logs 😉 This will be less of an issue once we are able to include tracer logs in the Agent Flare.

Ok, I've just saw the spdlog source, and it will throw an exception in the rotating sink .ctor, so I'll create a PR to handle that case.

@tonyredondo
Copy link
Member

Done here: #1028

@zacharycmontoya zacharycmontoya merged commit 0cee32f into master Nov 10, 2020
@zacharycmontoya zacharycmontoya deleted the zach/logs/improvements-latest branch November 10, 2020 22:35
@zacharycmontoya
Copy link
Collaborator Author

This is the docs PR to track logging changes: DataDog/documentation#9016

@kevingosse kevingosse added this to the 1.20.2 milestone Nov 24, 2020
macrogreg pushed a commit that referenced this pull request Aug 20, 2021
Makes planned improvements to the managed and native tracer logging:
- Both
  - Adds a new `DD_TRACE_LOG_DIRECTORY` environment variable to take precedence over `DD_TRACE_LOG_PATH`
- Native
  - Renames output file from `dotnet-profiler.log` to `dotnet-tracer-native.log`
  - Adds `Windows`/`Linux` to the native log output
  - Change the default behavior to only print out the Datadog environment variables that are set. When set to debug mode, the native logs will write all Datadog environment variables, including empty ones
- Managed
  - Adds unit tests for dynamically setting the managed tracer's log-level

One additional fix is made: Do not use the `EnvironmentHelpers` utility class in the `EnvironmentConfigurationSource` class. This creates an indirect dependency from the configuration system to the logging system.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:native-library Automatic instrumentation native C++ code (Datadog.Trace.ClrProfiler.Native) area:tests unit tests, integration tests area:tracer The core tracer library (Datadog.Trace, does not include OpenTracing, native code, or integrations) needs-docs-update
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants