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

API inconsistency between event and span argument order #456

Open
jonhoo opened this issue Nov 26, 2019 · 4 comments
Open

API inconsistency between event and span argument order #456

jonhoo opened this issue Nov 26, 2019 · 4 comments
Labels
crate/tracing Related to the `tracing` crate meta/breaking This is a breaking change, and should wait until the next breaking release.
Milestone

Comments

@jonhoo
Copy link
Contributor

jonhoo commented Nov 26, 2019

Feature Request

span! currently takes the "name" first, then fields, whereas event! takes the fields first then the "message". This means that you end up with the docs showing examples like:

span!(Level::TRACE, "login", user.name, user.email);
event!(
    Level::DEBUG,
    question.answer = answer,
    question.tricky = true,
    "the answer to {} is {}.", question, answer
);

The fact that the fields come before the string argument in span!, but after the string argument in event! is surprising, and will probably trip users up. It'd be good if the two matched up, even though their string arguments are slightly different.

@hawkw
Copy link
Member

hawkw commented Nov 26, 2019

so, the thing is...

span!(
    Level::DEBUG,
    "answering_question"
    question.answer = answer,
    question.tricky = true,
    "the answer to {} is {}.", question, answer
);

is also valid.

@jonhoo
Copy link
Contributor Author

jonhoo commented Nov 26, 2019

Could we perhaps use "special" syntax for a span's name somehow? To make it clear that it's something different from the message? Alternatively, can events also have names?

@hawkw hawkw added the crate/tracing Related to the `tracing` crate label Dec 18, 2019
@hawkw hawkw added the meta/breaking This is a breaking change, and should wait until the next breaking release. label Mar 11, 2020
@hawkw hawkw added this to the tracing 0.2 milestone Apr 8, 2020
@whatisaphone
Copy link

will probably trip users up

Can confirm, I am a tripped-up user! I'm here because I was wondering if span! didn't support the debug sigil ?. I got a misleading error message from span!(Level::INFO, field = ?value, "text").

I don't know the internals of tracing, but from my brief investigation it looks like hawkw's example sets the formatted string to a "magical" field named message, i.e. it's equivalent to this:

span!(
    Level::DEBUG,
    "answering_question",
    message = format!( "the answer to {} is {}.", question, answer),
    question.answer = answer,
    question.tricky = true,
);

I might have missed something, but I don't think that behavior is even mentioned in the docs.

I don't personally see the use for having both a name and a message as a default. I would propose binding format args to the name instead. That way, the syntax for event! and span! could be exactly the same. I think the consistency would be good.

@mladedav
Copy link
Contributor

I'd like to try to move this forward, so I've tried to compile the current state and list options for unifying the two macros.

Event

[name: <name>,][target: <target>,][parent: <parent>,] <level> [<fields>,][<fields.message>][, <fmt args for message>]

  • Only level is required (no specific part is required for level! macros)
  • name defaults to event <file>:<line>
  • target defaults to current module
  • parent defaults to current span or None if there's no span

Span

[target: <target>,][parent: <parent>,] <level> <name> [, <fields>][, <fields.message>][, <fmt args for message>]

  • Level and name are required (only name for level_span! macros.
  • target and parent default to the same values as event
  • name is required so has no default

Differences

The only difference is that the name is in another position and that it is required for spans.

When a single &'static str literal is supplied, it's interpreted as a name for spans and as a message for events.

The thing that is from my perception most often the point of confusion is that fields go before the message in events but after the name in spans (as illustrated in this issue).

People also seem to think that the span's name is equivalent to event's message and don't know there's also a formatted message after the fields in spans.

The last two points are my personal perceptions and I have no data to support that except for this issue and my experience so I might be wrong there.

Options

  • Use the same syntax as event! - each span would have to have span!(name: "something").
    • This would not be very ergonomic as everyone would have to write name: often for no reason other than unification.
    • This could be changed slightly so that the formatted argument is something like payload which could be interpreted differently by Subscribers based on whether it's an event or a span and maybe whether there's an explicit name.
      • We could use the same syntax but with different semantics for the fields. There's just clash with the explicit name.
      • This feels like too much magic unless there is a way to simplify this.
  • Use the same syntax as event! but don't require name for spans.
    • name should be easily set and most people will likely want to set it anyway so I don't think this would bring any benefits. Moreover, people might start using fields.message for span name instead for it's convenience.
  • Use the same syntax as span! with a required name.
    • Events often don't have a name and the message is more convenient as people can format the message.
  • Use new, different, unified syntax.
    • It should be easy to set name for spans and message for events, but as long as the syntax and semantics are the same, we would force users to either provide names for events or messages for spans.
  • Use new, different, unified syntax with different semantics for events and macros.
    • For example [target: <target>,][parent: <parent>,] <level> [<fields>,]<payload>[, <fmt args for payload>] where payload would be interpreted as message for events and a name for spans.
      • We couldn't set names for events then or there would be two ways to set names for spans or the unnamed field would behave differently based on whether name was provided.
      • We would either not require names for spans or we would require message for events.
  • Use different syntax for the two.
    • It's important usage of the two still remains as similar as possible, especially with regards to ordering of fields.
    • This allows us to use info_span!("name") and info!("message") at the same time.
    • This also allows us to require names for spans while not requiring anything specific for events.
    • We could move name after fields which would unify the usage for the most part. People wouldn't be tripped by fields being before the message in event! and after the name (which they often think is also the same thing as event's message or equivalent).
      • span! could for example have syntax like this: [target: <target>,][parent: <parent>,] <level> [<fields>,]<name>[, <fmt args for name>] - it's almost the same as the current event!, except there is no optional name at the beginning and the required potentially formatted field is used for span's name instead of for its message.

Current Questions

I think answering these could help to move this along:

  • Do we want to support both info_span!("name") and info!("message") as we do now, or are we willing to change either events to use its only argument as a name or spans to use its only argument as a message?
  • Do we want to use the formatted string for two different things in the two macros?
  • Can we create a good default name for spans? Is something like what events use (event <filename>:<line> sufficient? Name of the current function maybe?
  • Do we want to require name for spans? (we currently do)
  • Do we want to require message for events? (we currently don't)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
crate/tracing Related to the `tracing` crate meta/breaking This is a breaking change, and should wait until the next breaking release.
Projects
None yet
Development

No branches or pull requests

4 participants