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

Fix but in new_span for fmt layer #634

Merged
merged 3 commits into from
Mar 13, 2020
Merged

Fix but in new_span for fmt layer #634

merged 3 commits into from
Mar 13, 2020

Conversation

yaahc
Copy link
Collaborator

@yaahc yaahc commented Mar 13, 2020

Right now theres a bug where if you have an error layer and a fmt layer, and the error layer is first, the fmt layer will still fmt the args and will end up duplicating the output in the formatted fields.

This change correctly skips formatting fields if the fields already exist in the registry.

@hawkw
Copy link
Member

hawkw commented Mar 13, 2020

@yaahc the CI failure is not your fault; it looks like clippy changed something in 1.42 and is now complaining about something it used to not complain about; I opened #636 to fix it.

Copy link
Member

@hawkw hawkw left a comment

Choose a reason for hiding this comment

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

this looks good to me --- i had a style comment that isn't a blocker.

couple of notes before merging:

  • let's wait for subscriber: fix clippy lints #636 to merge and rebase onto master before merging this? that way, we can make sure CI passes on this branch before merging
  • can we make some minor changes to the commit message gudelines here? in particular, let's add subscriber: to the title when this merges. that will make writing the changelog less of a pain for me :)

tracing-subscriber/src/fmt/fmt_layer.rs Outdated Show resolved Hide resolved
@hawkw
Copy link
Member

hawkw commented Mar 13, 2020

@yaahc feel free to merge whenever

yaahc and others added 2 commits March 13, 2020 14:01
Co-Authored-By: Eliza Weisman <eliza@buoyant.io>
@yaahc yaahc merged commit bd35354 into master Mar 13, 2020
@yaahc yaahc deleted the fmtfix branch March 13, 2020 21:34
hawkw added a commit that referenced this pull request Apr 6, 2020
# 0.2.4 (April 6, 2020)

This release includes several API ergonomics improvements, including
shorthand constructors for many types, and an extension trait for
initializing subscribers using method-chaining style. Additionally,
several bugs in less commonly used `fmt` APIs were fixed.

### Added

- **fmt**: Shorthand free functions for constructing most types in `fmt`
  (including `tracing_subscriber::fmt()` to return a
  `SubscriberBuilder`, `tracing_subscriber::fmt::layer()` to return a
  format `Layer`, etc) (#660)
- **registry**: Shorthand free function `tracing_subscriber::registry()`
  to construct a new registry (#660)
- Added `SubscriberInitExt` extension trait for more ergonomic
  subscriber initialization (#660)
  
### Changed

- **fmt**: Moved `LayerBuilder` methods to `Layer` (#655)

### Deprecated

- **fmt**: `LayerBuilder`, as `Layer` now implements all builder methods
  (#655)
  
### Fixed

- **fmt**: Fixed `Compact` formatter not omitting levels with
  `with_level(false)` (#657)
- **fmt**: Fixed `fmt::Layer` duplicating the fields for a new span if
  another layer has already formatted its fields (#634)
- **fmt**: Added missing space when using `record` to add new fields to
  a span that already has fields (#659)
- Updated outdated documentation (#647)


Signed-off-by: Eliza Weisman <eliza@buoyant.io>
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.

2 participants