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

mock: document public APIs in event module #2426

Merged
merged 11 commits into from Jan 5, 2023
Merged

Conversation

hds
Copy link
Contributor

@hds hds commented Dec 27, 2022

Motivation

There has been interest around publishing tracing-mock to crates.io
for some time. In order to make this possible, documentation and some
code clean up is needed.

The event module needs documentation and examples.

Solution

This change adds documentation to the event module and all the public
APIs within it. This includes doctests on all the methods which serve as
examples.

The following pattern was applied to the description of most methods:

  • Short description of expectation
  • Additional clarification (where needed)
  • Description of cases that cause the expectation to fail
  • Examples
    • Successful validation
    • Unsuccesful validation

Two changes were also made in the text provided to the user when an
assertion fails for with_explicit_parent or with_contextual_parent.

One small API changes is also included:

The method in_scope has been placed behind the tracing-subscriber
feature flag as it currently only works with the MockSubscriber, not
with the MockCollector. If the feature flag is active and it is used
to set a non-empty scope, the MockCollector will panic with
unimplemented during validation.

Refs: #539

@hds hds requested review from hawkw, davidbarsky and a team as code owners December 27, 2022 14:21
This change adds documentation to the event module and all the public
APIs within it. This includes doctests on all the methods which serve as
examples.

Some small API changes are also included

The methods `with_explicit_parent` and `with_contextual_parent` have
had the negative case moved into separate methods
`without_explicit_parent` and `without_contextual_parent` respectively.
This removes the need for an `Option` around the parameter in the
original methods - which are likely to be the most used case (the
'without' case where `None` would have been passed before is not used
anywhere in this workspace).

The method `in_scope` has been placed behind the `tracing-subscriber`
feature flag as it currently only works with the `MockSubscriber`, not
with the `MockCollector`. If the feature flag is active and it is used
to set a non-empty scope, the `MockCollector` will panic with
`unimplemented` during validation.

Refs: #539
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!

I had some suggestions for improving the documentation here a little bit, let me know what you think!

Also, if you're willing to put a bit more time into this, it would be really nice if the docs had examples showing a case where an expectation should pass, and also showing a case where the expectation should fail. If that's too much extra work, though, don't worry about it.

tracing-mock/src/event.rs Outdated Show resolved Hide resolved
tracing-mock/src/event.rs Outdated Show resolved Hide resolved
tracing-mock/src/event.rs Show resolved Hide resolved
tracing-mock/src/event.rs Outdated Show resolved Hide resolved
tracing-mock/src/event.rs Outdated Show resolved Hide resolved
tracing-mock/src/event.rs Outdated Show resolved Hide resolved
tracing-mock/src/event.rs Outdated Show resolved Hide resolved
tracing-mock/src/event.rs Show resolved Hide resolved
tracing-mock/src/event.rs Outdated Show resolved Hide resolved
tracing-mock/src/event.rs Show resolved Hide resolved
These are mostly the simple changes and exclude the
with/without contextual/explicit parent topic which needs
a bit more work.

Co-authored-by: Eliza Weisman <eliza@buoyant.io>
@hds
Copy link
Contributor Author

hds commented Dec 28, 2022

Thanks!

I had some suggestions for improving the documentation here a little bit, let me know what you think!

Also, if you're willing to put a bit more time into this, it would be really nice if the docs had examples showing a case where an expectation should pass, and also showing a case where the expectation should fail. If that's too much extra work, though, don't worry about it.

Thank you so much for all the suggestions! Most of them I've applied directly.

For the topic around the contextual/explicit parents I'm going to prepare a separate commit, I also need to understand a bit the situation around a contextual parent, because your description doesn't match how I thought some of it worked!

I'm also more than happy to add in the negative tests. I already had it on my list of things to do, but had thought to put them into a separate test file (as opposed to in the examples) as I was worried that the API docs were getting too long. However I think you're right that they'd be better in the API docs as for a crate like tracing-mock that is likely where people will go to understand how to test what they want to test.

There is additional work needed to clean up `with_explicit_parent` and
`with_contextual_parent`, so they've been reverted back to how they are
in master. The comments from the PR review have been applied and both
functions have examples for expecting a parent span, expecting that the
event is a root, and a negative example of a failing expectation.
@hds hds requested a review from hawkw January 4, 2023 10:50
@hds
Copy link
Contributor Author

hds commented Jan 4, 2023

@hawkw I believe I've applied all the suggestions. Please let me know if there is anything further, I don't mind going through further review cycles with this, as many as we need.

There were copy-paste errors in the `with_explicit_parent` and
`with_contextual_parent` descriptions. The description of `in_scope` was
also improved and the note about it only working with `MockSubscriber`
was corrected to state that if used with `MockCollector` it will fail
directly (as opposed to not doing anything).
@hawkw
Copy link
Member

hawkw commented Jan 4, 2023

@hds thanks for the ping, I'll take a look now!

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 suggested some very minor edits to the documentation. Once those are addressed, I'll be happy to merge this!

tracing-mock/src/event.rs Outdated Show resolved Hide resolved
tracing-mock/src/event.rs Outdated Show resolved Hide resolved
tracing-mock/src/event.rs Outdated Show resolved Hide resolved
tracing-mock/src/event.rs Outdated Show resolved Hide resolved
tracing-mock/src/event.rs Outdated Show resolved Hide resolved
tracing-mock/src/event.rs Outdated Show resolved Hide resolved
tracing-mock/src/event.rs Outdated Show resolved Hide resolved
tracing-mock/src/event.rs Outdated Show resolved Hide resolved
tracing-mock/src/event.rs Outdated Show resolved Hide resolved
tracing-mock/src/event.rs Outdated Show resolved Hide resolved
Thanks Eliza!

Co-authored-by: Eliza Weisman <eliza@buoyant.io>
Copy link
Contributor Author

@hds hds left a comment

Choose a reason for hiding this comment

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

Changes applied. Thanks!

tracing-mock/src/event.rs Outdated Show resolved Hide resolved
hds added a commit that referenced this pull request Jan 5, 2023
Also cleaned up the text and tried to apply the code review comments
from #2426 to what I'd written already.
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 great, besides one last typo (sorry!). I'm gonna go ahead and merge it now!

tracing-mock/src/lib.rs Outdated Show resolved Hide resolved
@hawkw hawkw enabled auto-merge (squash) January 5, 2023 18:23
@hawkw hawkw merged commit dd67660 into master Jan 5, 2023
@hawkw hawkw deleted the hds/mock-docs-event branch January 5, 2023 18:36
@hds
Copy link
Contributor Author

hds commented Jan 5, 2023

Thank you for fixing that last typo! And thanks for all the very detailed reviews!

davidbarsky pushed a commit that referenced this pull request Sep 27, 2023
There has been interest around publishing tracing-mock to crates.io
for some time. In order to make this possible, documentation and some
code clean up is needed.

The `event` module needs documentation and examples.

This change adds documentation to the event module and all the public
APIs within it. This includes doctests on all the methods which serve as
examples.

The following pattern was applied to the description of most methods:
- Short description of expectation
- Additional clarification (where needed)
- Description of cases that cause the expectation to fail
- Examples
  - Successful validation
  - Unsuccesful validation

Two changes were also made in the text provided to the user when an
assertion fails for `with_explicit_parent` or `with_contextual_parent`.

One small API changes is also included:

The method `in_scope` has been placed behind the `tracing-subscriber`
feature flag as it currently only works with the `MockSubscriber`, not
with the `MockCollector`. If the feature flag is active and it is used
to set a non-empty scope, the `MockCollector` will panic with
`unimplemented` during validation.

Refs: #539
hawkw pushed a commit that referenced this pull request Oct 1, 2023
There has been interest around publishing tracing-mock to crates.io
for some time. In order to make this possible, documentation and some
code clean up is needed.

The `event` module needs documentation and examples.

This change adds documentation to the event module and all the public
APIs within it. This includes doctests on all the methods which serve as
examples.

The following pattern was applied to the description of most methods:
- Short description of expectation
- Additional clarification (where needed)
- Description of cases that cause the expectation to fail
- Examples
  - Successful validation
  - Unsuccesful validation

Two changes were also made in the text provided to the user when an
assertion fails for `with_explicit_parent` or `with_contextual_parent`.

One small API changes is also included:

The method `in_scope` has been placed behind the `tracing-subscriber`
feature flag as it currently only works with the `MockSubscriber`, not
with the `MockCollector`. If the feature flag is active and it is used
to set a non-empty scope, the `MockCollector` will panic with
`unimplemented` during validation.

Refs: #539
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