Uploaded image for project: 'HTrace'
  1. HTrace
  2. HTRACE-275

Add NullTracer which never creates a trace span

    Details

    • Type: Improvement
    • Status: Open
    • Priority: Major
    • Resolution: Unresolved
    • Affects Version/s: None
    • Fix Version/s: None
    • Component/s: api
    • Labels:
      None

      Description

      Masatake Iwasaki suggested adding a NullTracer which never creates a trace span. This would be more convenient than checking tracer objects against null in some cases.

      1. HTRACE-275.001.patch
        5 kB
        Colin P. McCabe

        Activity

        Hide
        cmccabe Colin P. McCabe added a comment -

        A few notable things:

        • We implement this via a subclass, but we don't make the subclass public (in case we want to switch to a different implementation later)
        • Can't "pull the rug out" from another thread by closing NullTracer... its close method does nothing.
        • Can't add Sampler objects to NullTracer
        • NullTracer is in its own SamplerPool to avoid causing issues for applications that iterate over the main sampler pool (like Hadoop once we add the ability to modify samplers via RPC)
        Show
        cmccabe Colin P. McCabe added a comment - A few notable things: We implement this via a subclass, but we don't make the subclass public (in case we want to switch to a different implementation later) Can't "pull the rug out" from another thread by closing NullTracer ... its close method does nothing. Can't add Sampler objects to NullTracer NullTracer is in its own SamplerPool to avoid causing issues for applications that iterate over the main sampler pool (like Hadoop once we add the ability to modify samplers via RPC)
        Hide
        iwasakims Masatake Iwasaki added a comment -

        Thanks for quick work, Colin P. McCabe.

          public boolean addSampler(Sampler sampler) {
            throw new RuntimeException("Can't add a new sampler to the NullTracer.");
          }
        

        I think NullTracer should not fail in the code path non-null Tracer does not fail. How about logging DEBUG message and just return false?

        Other part looks good to me.

        Show
        iwasakims Masatake Iwasaki added a comment - Thanks for quick work, Colin P. McCabe . public boolean addSampler(Sampler sampler) { throw new RuntimeException( "Can't add a new sampler to the NullTracer." ); } I think NullTracer should not fail in the code path non-null Tracer does not fail. How about logging DEBUG message and just return false? Other part looks good to me.
        Hide
        cmccabe Colin P. McCabe added a comment -

        Thanks for reviewing... and thanks for the original idea. I guess the rationale for having NullTracer#addSampler fail is that it seems like something the programmer should not be doing. In general, the Tracer#addSampler method is a "power" method, only used by power users like Hadoop that want to change the samplers that are active at runtime. I guess logging an ERROR message is one alternative to throwing an exception, but I feel like an exception might be appropriate here, since we're not able to do the thing that was requested.

        In general, I'm kind of concerned that NullTracer could hide errors where we actually didn't initialize the Tracer, and we thought we did. Perhaps NullTracer should log a debug message when it's used, to indicate that we didn't initialize things in the order we thought? Hmm...

        Show
        cmccabe Colin P. McCabe added a comment - Thanks for reviewing... and thanks for the original idea. I guess the rationale for having NullTracer#addSampler fail is that it seems like something the programmer should not be doing. In general, the Tracer#addSampler method is a "power" method, only used by power users like Hadoop that want to change the samplers that are active at runtime. I guess logging an ERROR message is one alternative to throwing an exception, but I feel like an exception might be appropriate here, since we're not able to do the thing that was requested. In general, I'm kind of concerned that NullTracer could hide errors where we actually didn't initialize the Tracer, and we thought we did. Perhaps NullTracer should log a debug message when it's used, to indicate that we didn't initialize things in the order we thought? Hmm...
        Hide
        iwasakims Masatake Iwasaki added a comment -

        Perhaps NullTracer should log a debug message when it's used, to indicate that we didn't initialize things in the order we thought?

        NullTracer should be used only when it is hard to make sure that Tracer is initialized in every code path, like FileSystem implementations in Hadoop. Users of Tracer#getNullTracer must be aware of it and I think noticing in documentation is enough. Once it is used, it could not be said to be always exceptional.

        In general, the Tracer#addSampler method is a "power" method, only used by power users like Hadoop that want to change the samplers that are active at runtime.

        Based on the fact that users of Tracer#getNullTracer must be aware of it's usecase, I think it is ok to throw exception in addSampler now. +1 on 001.

        Show
        iwasakims Masatake Iwasaki added a comment - Perhaps NullTracer should log a debug message when it's used, to indicate that we didn't initialize things in the order we thought? NullTracer should be used only when it is hard to make sure that Tracer is initialized in every code path, like FileSystem implementations in Hadoop. Users of Tracer#getNullTracer must be aware of it and I think noticing in documentation is enough. Once it is used, it could not be said to be always exceptional. In general, the Tracer#addSampler method is a "power" method, only used by power users like Hadoop that want to change the samplers that are active at runtime. Based on the fact that users of Tracer#getNullTracer must be aware of it's usecase, I think it is ok to throw exception in addSampler now. +1 on 001.
        Hide
        stack stack added a comment -

        If user calls wrap or getTracerPool, that is ok against NullTracer?

        A subclass is returning an instance of superclass?

        177 public static Tracer getNullTracer()

        { 178 return NullTracer.INSTANCE; 179 }

        Otherwise looks good.

        Show
        stack stack added a comment - If user calls wrap or getTracerPool, that is ok against NullTracer? A subclass is returning an instance of superclass? 177 public static Tracer getNullTracer() { 178 return NullTracer.INSTANCE; 179 } Otherwise looks good.
        Hide
        cmccabe Colin P. McCabe added a comment -

        If user calls wrap or getTracerPool, that is ok against NullTracer?

        NullTracer#wrap should work fine, since it's just using the wrapper classes that call newScope, and we have overridden that in NullScope. NullTracer#getTracerPool will return TracerPool.NULL_TRACER_POOL-- this is not the default tracer pool, but it is a valid tracer pool.

        A subclass is returning an instance of superclass... [in gettNullTracer]

        It's a static method, so this should be OK? The motivation for putting it in Tracer.java rather than NullTracer.java was because I didn't want to make NullTracer a public class (reduce API surface).

        I looked at the Hadoop code, and I don't think we really need this patch there. We fixed the null pointer exception we had earlier by correctly initializing the tracer variable. I'll have to think about the patch-- like I said, I'm a little worried that overuse of NullTracer in client code could cause us to lose spans. Maybe there are some use-cases I haven't thought about...

        Show
        cmccabe Colin P. McCabe added a comment - If user calls wrap or getTracerPool, that is ok against NullTracer? NullTracer#wrap should work fine, since it's just using the wrapper classes that call newScope , and we have overridden that in NullScope . NullTracer#getTracerPool will return TracerPool.NULL_TRACER_POOL -- this is not the default tracer pool, but it is a valid tracer pool. A subclass is returning an instance of superclass... [in gettNullTracer] It's a static method, so this should be OK? The motivation for putting it in Tracer.java rather than NullTracer.java was because I didn't want to make NullTracer a public class (reduce API surface). I looked at the Hadoop code, and I don't think we really need this patch there. We fixed the null pointer exception we had earlier by correctly initializing the tracer variable. I'll have to think about the patch-- like I said, I'm a little worried that overuse of NullTracer in client code could cause us to lose spans. Maybe there are some use-cases I haven't thought about...
        Hide
        stack stack added a comment -

        It's a static method, so this should be OK?

        Don't you feel dirty?

        ...was because I didn't want to make NullTracer a public class (reduce API surface).

        Make it internal?

        Show
        stack stack added a comment - It's a static method, so this should be OK? Don't you feel dirty? ...was because I didn't want to make NullTracer a public class (reduce API surface). Make it internal?
        Hide
        cmccabe Colin P. McCabe added a comment -

        What I like about the current patch is that we could remove the NullTracer class entirely and clients wouldn't know. For example, we could switch to having a boolean inside the tracer to determine whether it traced, or whatever. In the patch, NullTracer is purely an implementation detail rather than part of the API. To me this feels cleaner than exposing the NullTracer class. What feels dirty to me about NullTracer is that it feels kind of like a "fudge" to handle code that doesn't initialize itself properly. Maybe once we add tracing to a few more things we'll have more perspective on this and whether it's really necessary.

        Show
        cmccabe Colin P. McCabe added a comment - What I like about the current patch is that we could remove the NullTracer class entirely and clients wouldn't know. For example, we could switch to having a boolean inside the tracer to determine whether it traced, or whatever. In the patch, NullTracer is purely an implementation detail rather than part of the API. To me this feels cleaner than exposing the NullTracer class. What feels dirty to me about NullTracer is that it feels kind of like a "fudge" to handle code that doesn't initialize itself properly. Maybe once we add tracing to a few more things we'll have more perspective on this and whether it's really necessary.
        Hide
        iwasakims Masatake Iwasaki added a comment -

        ... I didn't want to make NullTracer a public class (reduce API surface).

        NullTracer could be private nested subclass of Tracer to limit visibility further.

        ... it feels kind of like a "fudge" to handle code that doesn't initialize itself properly.

        It is exactly for non-ideal world and we can say that it should not be used as far as possible in documentation. Adding tracing to software having long history may need this sometimes.

        Show
        iwasakims Masatake Iwasaki added a comment - ... I didn't want to make NullTracer a public class (reduce API surface). NullTracer could be private nested subclass of Tracer to limit visibility further. ... it feels kind of like a "fudge" to handle code that doesn't initialize itself properly. It is exactly for non-ideal world and we can say that it should not be used as far as possible in documentation. Adding tracing to software having long history may need this sometimes.

          People

          • Assignee:
            cmccabe Colin P. McCabe
            Reporter:
            cmccabe Colin P. McCabe
          • Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:

              Development