Page MenuHomePhabricator

[clangd] Changed tracing interfaces

Authored by ilya-biryukov on Nov 27 2017, 4:53 AM.



EventTracer interface now contains two methods:

  • spanEvent for events that have duration,
  • instant for events that are instant.

Diff Detail

Event Timeline

ilya-biryukov created this revision.Nov 27 2017, 4:53 AM
luckygeck added inline comments.Nov 27 2017, 5:26 AM

This is a (1)static non-pod member in an (2) interface. Is it really a good idea? If we plan to have only one ctxkey, then maybe let's make it not bound to EventTracer?




Maybe it is better to calculate these Args only if the tracing is enabled? This might be done at least this two ways:

  1. Have bool is_tracing_enabled() method that we can check before calculating args.
  2. Pass a function that will produce json::obj when called.
  • Fixed a typo.
ilya-biryukov marked an inline comment as done.Nov 27 2017, 6:47 AM
ilya-biryukov added inline comments.

It does not have any data members, has trivial default constructor and trivial destructor.
I don't think there are any problems we're gonna hit with this one, or am I missing something?

then maybe let's make it not bound to EventTracer?

Do you propose to move it out of the class into the clangd::trace namespace instead?


The current implementation won't compute args if EventTracer inside a Context is null, so I think this should cover our needs for per-request tracing (see SPAN_ATTACH macro).
But is_tracing_enabled() makes sense if we'd like to turn the tracing off in a middle of a single Context lifetime. This would make sense for global tracer, is this the use-case you anticipate?

luckygeck added inline comments.Nov 27 2017, 7:37 AM

I've just looked through the change adding Context and Key<>\PtrKey<> thingy. Yes, these classes are POD, so it is fine to have them static (as you rely only on their address).

Yes, I'd prefer to move the key out of class into a namespace - interface looks better without any data IMO.


Oh, ok - setting tracer in a context only when we need to looks good enough.

ilya-biryukov planned changes to this revision.Dec 5 2017, 9:00 AM

Have to update to match the new Context implementation.

  • Update the patch to accomodate changes from tracing and Context patches.
sammccall added inline comments.Dec 7 2017, 1:34 AM

why not?


just call this begin?
Otherwise style is beginEvent I think


How is identity represented between begin/end event? via Name doesn't seem robust, so why the need to pass it twice? Does providing different contexts for start/end make sense?

It might be cleaner/more flexible to have

std::function<void(json::obj&&)> begin()
and call the return value to signal end.

This seems likely to be pretty easy for different providers to implement, and is easy to use from Span.

ilya-biryukov added inline comments.Dec 8 2017, 3:12 AM

Because T must outlive the Span, so we can't really have if (!T) evaluate to different things in constructor and destructor.
Am I missing something?

sammccall added inline comments.Dec 8 2017, 3:25 AM

I was being disingenuous, I agree with you.
But can you change the message to "Changed tracer during a span"?

assert(Args) and "Args can't be null at this point" are basically synonyms :-)

  • Update the patch to accomodate changes from tracing and Context patches.
  • Updated tracing after changes to Context. We now store a clone() of Context in Span instead of ContextData.
  • Pass Context by const-ref instead of by ref.

Updated the patch after changes to Context

ilya-biryukov marked 3 inline comments as done.
  • Changed the EventTracer interface to return a callback from beginEvent instead of using begin_event/end_event method pairs.

The new name is beginSpan. Hope that sounds good.


Done. As discussed offline, the interface you propose seems much nicer.
Used names beginSpan and instant.

sammccall accepted this revision.Dec 13 2017, 8:16 AM
sammccall added inline comments.

I'm not sure this is useful. Tracers that aren't interested in args can't opt out by providing a null callback, unless they also don't care about the end time.
This seems unlikely (a null tracer, I guess)

This revision is now accepted and ready to land.Dec 13 2017, 8:16 AM
ilya-biryukov added inline comments.Dec 14 2017, 6:59 AM

One use-case that came to mind is turning the tracing on or off on per-request basis

ilya-biryukov edited the summary of this revision. (Show Details)Dec 14 2017, 7:08 AM

Merged with head

Closed by commit rCTE320708: [clangd] Changed tracing interfaces (authored by ibiryukov, committed by ). · Explain WhyDec 14 2017, 7:34 AM
This revision was automatically updated to reflect the committed changes.