Skip to content

feat: migrate logging log -> tracing #1720

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

Draft
wants to merge 64 commits into
base: main
Choose a base branch
from

Conversation

CommanderStorm
Copy link
Collaborator

@CommanderStorm CommanderStorm commented Mar 4, 2025

Lets split this up into two PRs

  • one changing the interface, tests and dependencys
  • one adding additional configuration options to martin AND documenting them

Alternatively, we can keep this PR open and I am going to get back to this sometime on the weekend

Resolves #836

we should continue using the RUST_LOG env var for all the configuration, with the possible addition of CLI and Config params

  • RUST_LOG
  • custom env variables MARTIN_LOG_FORMAT, MARTIN_LOG_LEVEL, ...
  • deduplicating code
  • CLI
  • Config
  • Writing docs (🦖)

Copy link
Member

@nyurik nyurik left a comment

Choose a reason for hiding this comment

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

needs a few cleanups, but this might be ok... We should probably run some perf tests (we have that using oha in the justfile iirc) - to see if there are any significant slowdowns due to the switch

@CommanderStorm CommanderStorm marked this pull request as draft March 6, 2025 02:52
@CommanderStorm
Copy link
Collaborator Author

CommanderStorm commented Mar 8, 2025

So I ran the performance benchmarks..
Seems like there is a supprising performance difference..

Will investigate where this is coming from.. I have some theories, such as my terminal not being the best at printing formatted output and such => will test them and report back and update the table below

experiment
baseline/log + with terminal output via ssh image image image
log + no terminal output image image image
tracing + with terminal output via ssh image image image
tracing + no terminal output image image image
tracing+ with terminal output via ssh + MARTIN_LOG_FORMAT=json image image image
tracing+ with terminal output via ssh + NO request logging via actix-tracing, but via actix-middleware + MARTIN_LOG_FORMAT=json image image image
tracing+ with terminal output via ssh + request logging via actix-tracing, but NOT via actix-middleware + MARTIN_LOG_FORMAT=json image image image
tracing + with terminal output via ssh + request logging via actix-tracing, but NOT via actix-middleware + MARTIN_LOG_FORMAT=json + interest cache in tracing_log image image image
tracing + no terminal output + request logging via actix-tracing, but NOT via actix-middleware + interest cache in tracing_log image image image

@CommanderStorm
Copy link
Collaborator Author

CommanderStorm commented Mar 8, 2025

Okay.. So I got performance up there again. Was a good call to do benchmarks 😅

I think there are severe performance issues with my ssh-terminal and rendering special characters => I am going to discount the initial and only consider MARTIN_LOG_FORMAT=json
I don't think on a server, this is reasonably a problem.

The rqs went down on some of these, but 170k -> 130k is likely not something that users will see in reality.
Will look further into what is causing the slowdown there

@CommanderStorm

This comment was marked as off-topic.

@CommanderStorm
Copy link
Collaborator Author

So apparently some of our dependencys log via tracing (switch on the trace level to see it.. that is a lot).
There is the expected change from not having so much semi-const stuff.. I think the ~relatively small change is fine..

=> I think performance is not a bottleneck anymore. Will investigate configurability next

@nyurik
Copy link
Member

nyurik commented Mar 8, 2025

WOW! This is a very thorough job you did there! Thank you!

Ideally, we should continue using the RUST_LOG env var for all the configuration, with the possible addition of CLI and Config params. If we need more values like output format (vs output filtering), that may go into separate params too.

When doing performance, stdout + term + ssh is almost always a bad choice - there might be locking and syscall issues affecting it. Best to redirect it to /dev/null to avoid additional uncertainty, or perhaps dump to a file (but that might also be less than stable).

#![doc = include_str!("../README.md")]

#[derive(Default)]
pub struct MartinObservability {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Bit unsure about the API/naming here..

I tried to make it less "magic", but unsure if this is the best way to handle it.

CommanderStorm and others added 5 commits March 11, 2025 15:03
Resolves maplibre#1475

One possible concern is that i32 is now used as i64 with sqlx. This
**might** impact `tile_xxh3_64_hash` value validity, but I don't see
conclusive evidence of that. Note that the tests were incorrectly dumped
as i32, thus had to be updated, but it does not seem to affect the
actual storage or validation...

---------

Co-authored-by: Frank Elsinga <[email protected]>
@@ -27,6 +27,10 @@ TEST_TEMP_DIR="$(dirname "$0")/mbtiles_temp_files"
rm -rf "$TEST_TEMP_DIR"
mkdir -p "$TEST_TEMP_DIR"

# by default, martin/.. do pretty up their output for terminals with colors
# in CI, while comparing outputs, this makes testcases less readable
export NO_COLOR=true
Copy link
Collaborator Author

@CommanderStorm CommanderStorm Mar 12, 2025

Choose a reason for hiding this comment

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

This SHOULD work, but for some reason it does only locally.. no clue why..

This also just SOMETIMES does this.. so weird

Will have to look further into the code what is overriding this being active...

@nyurik nyurik requested a review from Copilot March 27, 2025 15:03
Copy link
Contributor

@Copilot Copilot AI left a comment

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 migrates the logging functionality from the log crate to tracing throughout Martin’s codebase and updates the corresponding CLI help and documentation.

  • Replace log imports with tracing equivalents in multiple modules
  • Update CLI text and configuration documentation to reference tracing-subscriber instead of env_logger
  • Add a new martin-observability-utils package for improved observability support

Reviewed Changes

Copilot reviewed 71 out of 71 changed files in this pull request and generated no comments.

Show a summary per file
File Description
martin/src/pg/pg_source.rs Updated log::debug to tracing::debug
martin/src/pg/config_table.rs Replaced log macros with tracing macros
martin/src/pg/config.rs Replaced log::warn with tracing::warn
martin/src/pg/builder.rs Replaced log macros with tracing macros
martin/src/mbtiles/mod.rs Replaced log::trace with tracing::trace
martin/src/fonts/mod.rs Replaced log macros with tracing macros
martin/src/file_config.rs Replaced log macros with tracing macros
martin/src/config.rs Updated log::info to tracing::info
martin/src/cog/source.rs Reordered imports and replaced log::warn with tracing::warn
martin/src/args/srv.rs Updated use and derived order; added new CLI options for logging
martin/src/args/root.rs Updated after_help text with tracing-subscriber docs reference
martin/src/args/pg.rs Replaced log macros with tracing macros
martin/src/args/environment.rs Replaced log::warn with tracing::warn
martin/Cargo.toml Removed env_logger and log; added tracing dependencies
martin-observability-utils/src/lib.rs Introduced tracing-based observability utilities
martin-observability-utils/README.md Added initial README for logging utilities
martin-observability-utils/Cargo.toml Added package manifest for the observability utilities package
docs/src/env-vars.md Updated logging env var details to reference tracing-subscriber docs
docs/src/config-file.md Updated configuration examples to use tracing-based logging
Cargo.toml Updated workspace members and dependencies to include tracing-based crates
Comments suppressed due to low confidence (1)

martin-observability-utils/src/lib.rs:51

  • Consider clarifying this error message to more explicitly state that setting the global subscriber failed because a subscriber has already been set. For example, use: "Failed to set global subscriber: global subscriber already set."
        .expect("since martin has not set the global_default, no global default is set");

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.

Allow jsonl-style logging for servers
2 participants