Skip to content

fix(gui-client): apply log directives from MDM config - #9443

Merged
thomaseizinger merged 2 commits into
mainfrom
fix/apply-mdm-log-directives
Jun 6, 2025
Merged

fix(gui-client): apply log directives from MDM config#9443
thomaseizinger merged 2 commits into
mainfrom
fix/apply-mdm-log-directives

Conversation

@thomaseizinger

@thomaseizinger thomaseizinger commented Jun 6, 2025

Copy link
Copy Markdown
Member

When introducing the MDM config, we took into account the log directives from the config when applying it via the GUI but failed to apply the new directives on startup.

@thomaseizinger
thomaseizinger requested review from Copilot and jamilbk June 6, 2025 08:23
@vercel

vercel Bot commented Jun 6, 2025

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
firezone ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jun 6, 2025 8:27am

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull Request Overview

This PR applies MDM log directives to the GUI client by attempting to reload the logging configuration when provided by the MDM settings.

  • Added conditional logic to check for log directives and reload the logging configuration.
  • Logs an informational message if applying the directives fails.


if let Some(directives) = mdm_settings.log_filter.as_ref() {
if let Err(e) = reloader.reload(directives) {
tracing::info!(%directives, "Failed to apply MDM logging directives: {e:#}");

Copilot AI Jun 6, 2025

Copy link

Choose a reason for hiding this comment

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

[nitpick] Consider reviewing the choice of log level. If failing to apply log directives is a critical issue, it may be more appropriate to use a higher log level (e.g., warning or error) instead of informational.

Suggested change
tracing::info!(%directives, "Failed to apply MDM logging directives: {e:#}");
tracing::warn!(%directives, "Failed to apply MDM logging directives: {e:#}");

Copilot uses AI. Check for mistakes.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Logging this on INFO because we don't want Sentry alerts for it.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It would be nice if Sentry had logic to log a sampled %, or ignore the second and later logs for an event.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

There is sampling but it is not per event.

@thomaseizinger
thomaseizinger added this pull request to the merge queue Jun 6, 2025
auto-merge was automatically disabled June 6, 2025 14:10

Pull Request is not mergeable

Merged via the queue into main with commit caafcbc Jun 6, 2025
@thomaseizinger
thomaseizinger deleted the fix/apply-mdm-log-directives branch June 6, 2025 14:12
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.

3 participants