Skip to content

feat: add martin-tracing-utils for configuring tracing (1/3) #1747

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

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

CommanderStorm
Copy link
Collaborator

@CommanderStorm CommanderStorm commented Mar 12, 2025

This is the first PR to break out of #1720 to make it reviewable.

The purpose is mainly to see if the added utilities are

  • read & undaerstandable
  • not magic for others and
  • not too verbose
  • and the name martin-tracing-utils okay.

Current usage of this code would be

// since logging is not yet available, we have to manually check the locations
let log_filter = LogLevel::from_argument("--log-level")
    .or_in_config_file("--config", "log_level")
    .or_env_var("MARTIN_LOG_FORMAT")
    .lossy_parse_to_filter_with_default("martin=info");
let log_format = LogFormat::from_argument("--log-level")
    .or_in_config_file("--config", "log_format")
    .or_env_var("RUST_LOG")
    .or_default(LogFormatOptions::Compact);
MartinObservability::from((log_filter, log_format))
    .with_initialised_log_tracing()
    .set_global_subscriber();

@CommanderStorm CommanderStorm requested a review from nyurik March 25, 2025 02:08
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 introduces the new martin-tracing-utils library to configure tracing for the Martin tileserver. Key changes include:

  • Implementation of utilities to initialize and set a global tracing subscriber.
  • The addition of CLI, environment variable, and configuration file support for log directives and log format options.
  • Updates to workspace configuration and dependencies to integrate the new module.

Reviewed Changes

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

File Description
martin-tracing-utils/src/lib.rs Implements observability and logging format utilities with tracing integration.
martin-tracing-utils/README.md Provides documentation for the new tracing utility library.
martin-tracing-utils/Cargo.toml Defines package metadata and dependencies for the new module.
Cargo.toml Updates workspace members and adds necessary tracing dependencies.
Comments suppressed due to low confidence (3)

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

  • Consider rephrasing the error message in set_global_subscriber for improved clarity, for example: 'Failed to set global subscriber: global subscriber has already been set.'
.expect("since martin has not set the global_default, no global default is set");

martin-tracing-utils/src/lib.rs:115

  • [nitpick] Consider using structured logging or a more user-friendly error reporting mechanism instead of eprintln!, and clearly list the accepted log format values.
eprintln!("Ignoring specified cli argument {argument} {v} as it is not a valid log format. Can be one of full, compact, bare, pretty, json");

martin-tracing-utils/src/lib.rs:150

  • [nitpick] Consider using a structured logging approach here as well to report invalid log format values from the environment variable, and ensure the message clearly outlines the correct options.
eprintln!("Ignoring specified environment variable {key}={v} as it is not a valid log format. Can be one of full, compact, bare, pretty, json");

@sharkAndshark sharkAndshark requested a review from Copilot May 6, 2025 02:45
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 introduces a new utility library, martin-tracing-utils, to configure and initialize tracing for the Martin tile server. Key changes include the implementation of log format and log level extraction from CLI arguments, config file, and environment variables; setting up the global tracing subscriber; and associated tests and documentation updates.

Reviewed Changes

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

File Description
martin-tracing-utils/src/lib.rs New tracing utilities implementation including log format and level APIs
martin-tracing-utils/README.md Documentation for the new library with badges and basic usage instructions
martin-tracing-utils/Cargo.toml Package metadata and dependency setup for the tracing utilities
Cargo.toml Added the new martin-tracing-utils member to the workspace and dependency updates
Comments suppressed due to low confidence (2)

martin-tracing-utils/src/lib.rs:239

  • The variable name 'traversion' appears to be a misspelling; consider renaming it to 'traversal' or another more descriptive name for clarity.
for traversion in key_parts {

martin-tracing-utils/README.md:3

  • The documentation badge references 'martin-observability-utils' while the package is named 'martin-tracing-utils'; consider updating the badge link to ensure consistency.
[![docs.rs docs](https://docs.rs/martin-observability-utils/badge.svg)](https://docs.rs/martin-observability-utils)

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.

1 participant