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.

Sign up for GitHub

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

Jump to bottom

subscriber: Rename Layer::new_span method for consistency #630

Closed
yaahc opened this issue Mar 11, 2020 · 4 comments · Fixed by #1674
Closed

subscriber: Rename Layer::new_span method for consistency #630

yaahc opened this issue Mar 11, 2020 · 4 comments · Fixed by #1674
Labels
crate/subscriber Related to the `tracing-subscriber` crate meta/breaking This is a breaking change, and should wait until the next breaking release.
Milestone
tracing-subscribe...

Comments

@yaahc
Copy link
Collaborator

yaahc commented Mar 11, 2020

Right now the four event handling fns spans in the Layer trait are called

on_exit, on_enter, on_close, and new_span

new_span breaks the pattern set by the other methods and can be a little surprising / confusing, I spent more than a few seconds trying to find a method called on_open.

Next time we are making breaking changes we should consider renaming new_span to on_new_span or on_open.
@hawkw hawkw changed the title Rename new_span method for consistency subscriber: Rename new_span method for consistency Mar 11, 2020
@hawkw hawkw changed the title subscriber: Rename new_span method for consistency subscriber: Rename Layer::new_span method for consistency Mar 11, 2020
@hawkw
Copy link
Member

hawkw commented Mar 11, 2020

👍 we could also start this in a non-breaking change by adding a separate on_new_span/on_open method and deprecating new_span (and having on_open call new_span by default, to avoid breaking existing implementations). However, the default changes would take a little thinking about to make sure we don't introduce weird behavior by accident...

BTW: I edited the issue title a little to make it clearer that this is about changing the Layer::new_span method, not Subscriber::new_span in tracing-core.

@hawkw hawkw added the crate/subscriber Related to the `tracing-subscriber` crate label Mar 11, 2020
@hawkw hawkw added this to the tracing-subscriber 0.3 milestone Mar 11, 2020
@hawkw hawkw added the meta/breaking This is a breaking change, and should wait until the next breaking release. label Mar 11, 2020
@bnjjj
Copy link
Contributor

bnjjj commented Apr 2, 2020

Hi @hawkw I would like to make a PR for this. Is it possible ?

For the naming I prefer on_new_span but let me know what is your preference. And my plan is to update the trait making as you said the new_span method deprecated and create on_new_span method and call the previous one inside. I also plan to update all the implementation of layer to use the new method. What do you think ? Let me know if I forget something.

@hawkw
Copy link
Member

hawkw commented Apr 2, 2020

@bnjjj so, we would definitely welcome a PR. However, I should let you know that there is one issue with this ticket in particular. This would require a breaking change, and, in order to limit the frequency of breaking releases, we try to wait until we have a large number of breaking changes to make before publishing a breaking change release (in this case, tracing-subscriber 0.3.0).

So, while we'd welcome a PR, it may not be able to merge until we're ready to publish 0.3.0. This may take some time, since we would need to prepare other potential breaking changes. This means that if code that your PR changes is changed in other PRs as well, you'd have to update it when we're ready to merge. Just thought you should be aware of that complication. Thanks for your interest in contributing!

@bnjjj
Copy link
Contributor

bnjjj commented Apr 2, 2020

Seems good for me. I will take the point and update PR further if needed.

bnjjj added a commit to bnjjj/tracing that referenced this issue Apr 3, 2020
@bnjjj
refactor(subsriber): rename Layer::new_span method to Layer::on_new_s…
6ee4c63 
…pan for consistency tokio-rs#630

Signed-off-by: Benjamin Coenen <5719034+bnjjj@users.noreply.github.com>
@bnjjj bnjjj mentioned this issue Apr 3, 2020
Closed
hawkw added a commit that referenced this issue Oct 22, 2021
@hawkw
subscriber: rename Subscribe::new_span to on_new_span
303dbd0 
While we're breaking things, we may as well do this as well.

Closes #630
Closes #662
@hawkw hawkw mentioned this issue Oct 22, 2021
Merged
hawkw added a commit that referenced this issue Oct 22, 2021
@hawkw
subscriber: rename Subscribe::new_span to on_new_span (#1674)
2b3aa06 
While we're breaking things, we may as well do this as well.

Closes #630
Closes #662
hawkw added a commit that referenced this issue Oct 22, 2021
@hawkw
subscriber: rename Layer::new_span to on_new_span (#1674)
3566804 
While we're breaking things, we may as well do this as well.

Closes #630
Closes #662
hawkw added a commit that referenced this issue Oct 22, 2021
@hawkw
subscriber: rename Layer::new_span to on_new_span (#1674)
3f05ed6 
While we're breaking things, we may as well do this as well.

Closes #630
Closes #662
hawkw added a commit that referenced this issue Oct 22, 2021
@hawkw
subscriber: rename Layer::new_span to on_new_span (#1674)
8777d2c 
While we're breaking things, we may as well do this as well.

Closes #630
Closes #662
kaffarell pushed a commit to kaffarell/tracing that referenced this issue May 22, 2024
@hawkw @kaffarell
subscriber: rename Layer::new_span to on_new_span (tokio-rs#1674)
6386a7c 
While we're breaking things, we may as well do this as well.

Closes tokio-rs#630
Closes tokio-rs#662
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
crate/subscriber Related to the `tracing-subscriber` crate meta/breaking This is a breaking change, and should wait until the next breaking release.
Projects
None yet
Milestone
tracing-subscriber 0.3
3 participants
@yaahc @hawkw @bnjjj