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

Consistently pass a ref to a Dispatch #1045

Closed
wants to merge 4 commits into from
Closed

Consistently pass a ref to a Dispatch #1045

wants to merge 4 commits into from

Conversation

dvdplm
Copy link
Contributor

@dvdplm dvdplm commented Oct 16, 2020

This changes set_global_default to take a reference to its Dispatch for a more consistent API at the cost of a one-time clone when initializing the GLOBAL_DISPATCH static.

Closes #455
Part of #922

@dvdplm
Consistently pass a ref to a Dispatch
24ecaa2 
This changes `set_global_default` to take a reference to its `Dispatch` for a more consistent API at the cost of a one-time clone when initializing the `GLOBAL_DISPATCH` static.

Closes tokio-rs#455
Part of tokio-rs#922
@dvdplm
Merge remote-tracking branch 'upstream/master' into dp-always-pass-di…
5ad3ab1 
…spatch-as-ref-tokio-rs#455

* upstream/master: (28 commits)
  opentelemetry: prepare for 0.8.0 release (tokio-rs#1036)
  docs: add favicon for extra pretty docs (tokio-rs#1033)
  subscriber: fix `reload` ergonomics (tokio-rs#1035)
  chore(deps): update crossbeam-channel requirement from 0.4.2 to 0.5.0 (tokio-rs#1031)
  opentelemetry: Assign default ids if missing (tokio-rs#1027)
  chore: remove deprecated add-path from CI (tokio-rs#1026)
  attributes:  fix `#[instrument(err)]` in case of early returns (tokio-rs#1006)
  core: remove mandatory liballoc dependency with no-std (tokio-rs#1017)
  chore(deps): update cfg-if requirement from 0.1.10 to 1.0.0 (tokio-rs#1023)
  subscriber: warn if trying to enable a statically disabled level (tokio-rs#990)
  subscriber: use macros for module declarations (tokio-rs#1009)
  chore: remove `stdlib.rs` (tokio-rs#1008)
  core: fix linked list tests reusing `Registration`s (tokio-rs#1016)
  subscriber: support dash in target names (tokio-rs#1012)
  docs: switch to intra-doc links in tracing-core (tokio-rs#1010)
  tracing-opentelemetry: implement additional record types (bool, i64, u64) (tokio-rs#1007)
  core: add intrusive linked list for callsite registry (tokio-rs#988)
  serde: allow tracing-serde to work on no_std. (tokio-rs#960)
  tracing: remove `Into<Option<Id>>` impl for `Span` (tokio-rs#1003)
  tracing: make `Entered` `!Send` (tokio-rs#1001)
  ...
@dvdplm
Clone once
521cf74
@dvdplm dvdplm requested review from hawkw and a team as code owners October 16, 2020 08:08
@hawkw
Copy link
Member

hawkw commented Oct 18, 2020

I wonder if, rather than taking a &Dispatch here, it might be better to change the other functions to take a Dispatch by value, rather than taking one by reference. Taking the dispatcher by reference will always clone it, even if the owned value at the callsite isn't used anywhere else. In the set_global_default case, this doesn't matter, because this function is only ever called once, but set_default and with_default might be called multiple times over the course of a program's lifetime. It might be better to permit users to avoid the Arc clone in the case where the Dispatch is owned and won't be used again, and have them manually clone it otherwise?

@dvdplm
Copy link
Contributor Author

dvdplm commented Oct 18, 2020

It might be better to permit users to avoid the Arc clone in the case where the Dispatch is owned and won't be used again, and have them manually clone it otherwise?

Happy to explore that diection; I'll open a second PR and we can compare. :)

dvdplm added a commit to dvdplm/tracing that referenced this pull request Oct 19, 2020
@dvdplm
Pass Dispatch as owned value
c2fa02e 
As an alternative to tokio-rs#1045, this PR switches `set_default` and `with_default` to take an owned `Dispatch` instead of a ref.

See [comment here](tokio-rs#1045 (comment)) for motivation.

Closes tokio-rs#455
Part of tokio-rs#922
@dvdplm dvdplm mentioned this pull request Oct 19, 2020
Open
@dvdplm
Merge remote-tracking branch 'upstream/master' into dp-always-pass-di…
41dbed4 
…spatch-as-ref-tokio-rs#455

* upstream/master:
  subscriber: remove TraceLogger (tokio-rs#1052)
  subscriber: make Registry::enter/exit much faster (tokio-rs#1058)
  chore(deps): update env_logger requirement from 0.7 to 0.8 (tokio-rs#1050)
  chore: fix tracing-macros::dbg (tokio-rs#1054)
  chore(deps): update pin-project requirement from 0.4 to 1.0 (tokio-rs#1038)
  chore: remove duplicated section from tracing/README.md (tokio-rs#1046)
@dvdplm
Copy link
Contributor Author

dvdplm commented Oct 28, 2020

Closing as this is superseded by #1051

@dvdplm dvdplm closed this Oct 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hawkw hawkw Awaiting requested review from hawkw hawkw is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Consistency between various *default methods
2 participants
@dvdplm @hawkw