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

tracing-core: implement PartialEq, Eq for Metadata, FieldSet #2229

Merged
merged 2 commits into from Jul 19, 2022

Conversation

jswrenn
Copy link
Contributor

@jswrenn jswrenn commented Jul 19, 2022

This change manually re-implements PartialEq and Eq for
Metadata and FieldSet to define their equality strictly in
terms of callsite equality. In debug builds, the equality of
these types' other fields is also checked.

Documentation is added to both Metadata and FieldSet
explaining this behavior. The expectation that, in a well-behaving
application, Metadataand FieldSets with equal callsites will be
otherwise equal is documented on Callsite::metadata. This is not
a breaking change, as previous releases did not define equality for
Metadata or FieldSet. The Callsite trait remains safe, as this
expectation is not (yet) a safety-critical property.

This change improves the ergonomics of comparing Metadata, which
is very useful for testing and user-facing examples.

A `FieldSet` is equal to another `FieldSet` if they share the same
callsite and fields (provided in the same order). This ensures
that a `Field` applicable to one `FieldSet` is applicable to any
equal `FieldSet`. A `Metadata` is equal to another `Metadata` if
all of their contained metadata is exactly equal.
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.

i'm fine with this change, modulo some minor docs nits. thank you!

tracing-core/src/field.rs Show resolved Hide resolved
@@ -57,6 +46,7 @@ use core::{
/// [module path]: Self::module_path
/// [collector]: super::collect::Collect
/// [callsite identifier]: super::callsite::Identifier
#[derive(Eq, PartialEq)]
Copy link
Member

Choose a reason for hiding this comment

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

we may want a hand-written equality implementation for Metadatas so that we can compare the callsite identifier first, and short-circuit so that we don't have to do a bunch of string equality. i believe the derived PartialEq impl will compare fields in the order that they are declared.

that change could be made later as an optimization, though.

Copy link
Member

Choose a reason for hiding this comment

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

actually, i believe the derived implementations also generate an implementation of the StructuralPartialEq trait, which allows matching a type as a match pattern (which is possible in v0.1.x of tracing-core, where Metadata's fields are pub). this means that a change from a hand-written implementation to a derived one is potentially a breaking change...

tracing-core/src/metadata.rs Show resolved Hide resolved
This change manually re-implements `PartialEq` and `Eq` for
`Metadata` and `FieldSet` to define their equality strictly in
terms of callsite equality. In debug builds, the equality of
these types' other fields is also checked.

Documentation is added to both `Metadata` and `FieldSet`
explaining this behavior.

The expectation that, in a well-behaving application, `Metadata`
and `FieldSet`s with equal callsites will be otherwise equal is
documented on `Callsite::metadata`. This is not a breaking change,
as previous releases did not define equality for `Metadata` or
`FieldSet`. The `Callsite` trait remains safe, as this expectation
is not (yet) a safety-critical property.
@jswrenn
Copy link
Contributor Author

jswrenn commented Jul 19, 2022

@hawkw I went a little beyond your comments, and re-implemented PartialEq and Eq such that Metadata and FieldSets with equal callsites are assumed to be otherwise completely identical. (In debug releases, their other fields are still checked for equality.) These changes are accompanied by documentation edits to Metadata, FieldSet and Callsite. Let me know if you think this is reasonable.

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.

thanks, this looks good to me --- i think the hand-written equality implementations seem correct and should hopefully be more efficient than the derived ones.

thanks again for your work on this!

@hawkw hawkw merged commit 7ecaedf into tokio-rs:master Jul 19, 2022
hawkw pushed a commit that referenced this pull request Jul 20, 2022
A `FieldSet` is equal to another `FieldSet` if they share the same
callsite and fields (provided in the same order). This ensures
that a `Field` applicable to one `FieldSet` is applicable to any
equal `FieldSet`. A `Metadata` is equal to another `Metadata` if
all of their contained metadata is exactly equal.

This change manually re-implements `PartialEq` and `Eq` for
`Metadata` and `FieldSet` to define their equality strictly in
terms of callsite equality. In debug builds, the equality of
these types' other fields is also checked.

Documentation is added to both `Metadata` and `FieldSet`
explaining this behavior.

The expectation that, in a well-behaving application, `Metadata`
and `FieldSet`s with equal callsites will be otherwise equal is
documented on `Callsite::metadata`. This is not a breaking change,
as previous releases did not define equality for `Metadata` or
`FieldSet`. The `Callsite` trait remains safe, as this expectation
is not (yet) a safety-critical property.
hawkw pushed a commit that referenced this pull request Jul 20, 2022
A `FieldSet` is equal to another `FieldSet` if they share the same
callsite and fields (provided in the same order). This ensures
that a `Field` applicable to one `FieldSet` is applicable to any
equal `FieldSet`. A `Metadata` is equal to another `Metadata` if
all of their contained metadata is exactly equal.

This change manually re-implements `PartialEq` and `Eq` for
`Metadata` and `FieldSet` to define their equality strictly in
terms of callsite equality. In debug builds, the equality of
these types' other fields is also checked.

Documentation is added to both `Metadata` and `FieldSet`
explaining this behavior.

The expectation that, in a well-behaving application, `Metadata`
and `FieldSet`s with equal callsites will be otherwise equal is
documented on `Callsite::metadata`. This is not a breaking change,
as previous releases did not define equality for `Metadata` or
`FieldSet`. The `Callsite` trait remains safe, as this expectation
is not (yet) a safety-critical property.
hawkw pushed a commit that referenced this pull request Jul 20, 2022
A `FieldSet` is equal to another `FieldSet` if they share the same
callsite and fields (provided in the same order). This ensures
that a `Field` applicable to one `FieldSet` is applicable to any
equal `FieldSet`. A `Metadata` is equal to another `Metadata` if
all of their contained metadata is exactly equal.

This change manually re-implements `PartialEq` and `Eq` for
`Metadata` and `FieldSet` to define their equality strictly in
terms of callsite equality. In debug builds, the equality of
these types' other fields is also checked.

Documentation is added to both `Metadata` and `FieldSet`
explaining this behavior.

The expectation that, in a well-behaving application, `Metadata`
and `FieldSet`s with equal callsites will be otherwise equal is
documented on `Callsite::metadata`. This is not a breaking change,
as previous releases did not define equality for `Metadata` or
`FieldSet`. The `Callsite` trait remains safe, as this expectation
is not (yet) a safety-critical property.
hawkw added a commit that referenced this pull request Jul 29, 2022
# 0.1.29 (July 29, 2022)

This release of `tracing-core` adds `PartialEq` and `Eq` implementations
for metadata types, and improves error messages when setting the global
default subscriber fails.

### Added

- `PartialEq` and `Eq` implementations for `Metadata` ([#2229])
- `PartialEq` and `Eq` implementations for `FieldSet` ([#2229])

### Fixed

- Fixed unhelpful `fmt::Debug` output for
  `dispatcher::SetGlobalDefaultError` ([#2250])
- Fixed compilation with `-Z minimal-versions` ([#2246])

Thanks to @jswrenn and @CAD97 for contributing to this release!

[#2229]: #2229
[#2246]: #2246
[#2250]: #2250
hawkw added a commit that referenced this pull request Jul 29, 2022
# 0.1.29 (July 29, 2022)

This release of `tracing-core` adds `PartialEq` and `Eq` implementations
for metadata types, and improves error messages when setting the global
default subscriber fails.

### Added

- `PartialEq` and `Eq` implementations for `Metadata` ([#2229])
- `PartialEq` and `Eq` implementations for `FieldSet` ([#2229])

### Fixed

- Fixed unhelpful `fmt::Debug` output for
  `dispatcher::SetGlobalDefaultError` ([#2250])
- Fixed compilation with `-Z minimal-versions` ([#2246])

Thanks to @jswrenn and @CAD97 for contributing to this release!

[#2229]: #2229
[#2246]: #2246
[#2250]: #2250
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.

None yet

2 participants