subscriber: explain why we always call inner.register_callsite() before if statement

[Folyd]

I'm sure we can avoid unnecessary self.inner.register_callsite() call in these two Layered implementations.

[hawkw]

I don't think this is correct. register_callsite is not just responsible for performing filtering, it's also responsible for notifying the collector of the existence of the callsite --- it may choose to, for example, store some data for that callsite. This change means that if the outer Subscriber returns Interest::sometimes, the inner subscriber will never have the opportunity to register a callsite that is enabled.

If the subscriber, for example, preallocates storage for spans originating at particular callsites, it may then see a span from a callsite that it never had the opportunity to prepare storage for. The intention behind calling inner.register_callsite before the if statement was to ensure that the inner subscriber is informed that the callsite exists regardless of the outer subscriber's filtering decision. So, I think this change would actually introduce a bug.

Perhaps a better solution is to add a comment on that line explaining why we always call register_callsite on inner?

[Folyd]

I don't think this is correct. register_callsite is not just responsible for performing filtering, it's also responsible for notifying the collector of the existence of the callsite --- it may choose to, for example, store some data for that callsite. This change means that if the outer Subscriber returns Interest::sometimes, the inner subscriber will never have the opportunity to register a callsite that is enabled.

If the subscriber, for example, preallocates storage for spans originating at particular callsites, it may then see a span from a callsite that it never had the opportunity to prepare storage for. The intention behind calling inner.register_callsite before the if statement was to ensure that the inner subscriber is informed that the callsite exists regardless of the outer subscriber's filtering decision. So, I think this change would actually introduce a bug.

Perhaps a better solution is to add a comment on that line explaining why we always call register_callsite on inner?

Thanks, @hawkw. You're right. I have updated the comments. ❤️

[hawkw]

lgtm, thanks!