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

Enable global file logging #2371

Merged
merged 22 commits into from
Jul 23, 2024
Merged

Enable global file logging #2371

merged 22 commits into from
Jul 23, 2024

Conversation

Mpdreamz
Copy link
Member

@Mpdreamz Mpdreamz commented Jun 3, 2024

This ensures we can specify file logging globally and uniformly. Greatly simplifying the debug-ability of the agent.

The preferred way is to set any or all of the following:

  • ELASTIC_OTEL_LOG_TARGETS (to anything but none)
    • if only set to stdout no file will be created but global logging will kicking )
  • OTEL_DOTNET_AUTO_LOG_DIRECTORY
  • OTEL_LOG_LEVEL
    • trace debug will enable global file logging if the other two variables are not set explicitly.

This ensure we have one way to debug both our proprietary agent as well as the Elastic OpenTelemetry Distribution.

See: elastic/elastic-otel-dotnet#129

For backwards compatible reasons the profiler variables are also supported:

  • ELASTIC_APM_PROFILER_LOG
  • ELASTIC_APM_PROFILER_LOG_DIR
  • ELASTIC_APM_PROFILER_LOG_TARGETS

Globally setting

  • ELASTIC_APM_LOG_LEVEL and ELASTIC_APM_LOG_DIRECTORY is also supported but not preferred.

Setting these in any of our supported deploy scenarios:

  • Manual instrumentation (nuget)
    • ASP.NET (classic)
    • ASP.NET core
      • NOTE: we now log to both the configured ILogger and our global logging.
  • Auto Instrumentation
    • Profiler
    • Startup hooks
      To keep support existing deploys this now always globally logs to file if only ELASTIC_APM_STARTUP_HOOKS_LOGGING is set as well.

This further updates our docs for the profiler and troubleshooting to prefer ELASTIC_OTEL_* variables.

The profiler is updated to read the same environment variables as managed code.

@Mpdreamz Mpdreamz marked this pull request as ready for review June 3, 2024 18:06
Copy link
Contributor

@stevejgordon stevejgordon left a comment

Choose a reason for hiding this comment

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

A couple of minor fix suggestions. The only thing I'm a little unsure about is using the ELASTIC_OTEL_ env vars for this agent. I like that migration is easier, but not sure if it's subtly confusing why OTEL is mentioned in their names.

src/Elastic.Apm/AgentComponents.cs Outdated Show resolved Hide resolved
docs/troubleshooting.asciidoc Outdated Show resolved Hide resolved
docs/troubleshooting.asciidoc Outdated Show resolved Hide resolved
@Mpdreamz
Copy link
Member Author

Mpdreamz commented Jun 4, 2024

A couple of minor fix suggestions. The only thing I'm a little unsure about is using the ELASTIC_OTEL_ env vars for this agent. I like that migration is easier, but not sure if it's subtly confusing why OTEL is mentioned in their names.

Aye, it's not ideal but I'd love to be in a situation where we can point customers to 3 variables that work across the board and in mixed environments.

We could use:

  • ELASTIC_APM_LOG_LEVEL
  • ELASTIC_APM_LOG_DIRECTORY
  • ELASTIC_APM_LOG_TARGETS

For both? Or:

  • ELASTIC_APM_FILE_LOG_LEVEL
  • ELASTIC_APM_FILE_LOG_DIRECTORY
  • ELASTIC_APM_LOG_TARGETS

@stevejgordon
Copy link
Contributor

Yeah, okay. If we want one set to work for all things, let's go with the OTEL-based names, as that's where we expect to end up eventually. Would these make sense, since log level can apply to other loggers that may not be file-based?

  • ELASTIC_OTEL_LOG_LEVEL
  • ELASTIC_OTEL_FILE_LOG_DIRECTORY
  • ELASTIC_OTEL_LOG_TARGETS

@Mpdreamz
Copy link
Member Author

Mpdreamz commented Jun 4, 2024

Yeah it might make sense ELASTIC_OTEL_FILE_LOG_LEVEL on its own is enough to start logging to file today.

ELASTIC_OTEL_LOG_LEVEL on its own might be slightly confusing because the default for ELASTIC_OTEL_LOG_TARGETS is to include file.

However I think its benefits negate the possible confusion.

If we take this further we can also rename ELASTIC_OTEL_FILE_LOG_DIRECTORY to ELASTIC_OTEL_LOG_DIRECTORY since this only makes sense to file logging anyhow.

I'll update the PR to the following set:

  • ELASTIC_OTEL_LOG_LEVEL
  • ELASTIC_OTEL_LOG_DIRECTORY
  • ELASTIC_OTEL_LOG_TARGETS

I like symmetry in the naming and its more terser 😸

stevejgordon
stevejgordon previously approved these changes Jun 4, 2024
Copy link
Contributor

@stevejgordon stevejgordon left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@stevejgordon stevejgordon left a comment

Choose a reason for hiding this comment

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

One comment and observation.

stevejgordon
stevejgordon previously approved these changes Jul 18, 2024
Copy link
Contributor

@stevejgordon stevejgordon left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@stevejgordon stevejgordon left a comment

Choose a reason for hiding this comment

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

LGTM

@Mpdreamz Mpdreamz merged commit 40bf601 into main Jul 23, 2024
10 checks passed
@Mpdreamz Mpdreamz deleted the feature/external-logging-config branch July 23, 2024 08:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Simplify log configuration variables and include docs for no-code troubleshooting
2 participants