add adjuster property to always apply timestamp and duration if missing

[naoman]

PR for #25
Adding a property to Finagle adjuster to allow applying timestamp and duration without looking at the binary annotations. This rectifies the finagle Span model issue for non-finagle libraries that use finagle model for instrumentation.

[naoman]

Property name is not very intuitive. Any suggestions?

[naoman]

cc @adriancole

[codefromthecrypt]

This sounds like a different adjuster to me since it hasnt to do with finagle. Thats probably why it sounds unintuitive. You could put this in a new adjuster class the default module since it is generic, maybe note the issue on openzipkin.github.io about timestamp and duration for context since we usually shouldnt be doing this. For naming, try to follow the same name for this adjuster as the timestamp property in finagle so that when you read down the config it looks sensible. Sound good?

…

On 15 Mar 2017 00:30, "Naoman Abbas" ***@***.***> wrote: cc @adriancole <https://github.com/adriancole> — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#36 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAD61-2Tbi6mxPySxx2c623yNeBmQ3hZks5rlxUHgaJpZM4Mb7hc> .

[naoman]

I think we should keep this in the same adjuster class (FinagleAdjuster) for following two reasons:

1. We are making an adjustment for Finagle Span model shortcomings, so anyone who was using Finagle Span model for library instrumentation will need this.
2. The adjustment is exactly same as the "apply-timestamp-and-duration" adjustment, just the condition to apply is different. So its better to reuse the code, instead of having multiple copies.

[codefromthecrypt]

You are assuming that the lack of timestamp and duration is finagle centric when we know with certainty it is not. We cataloged many independent projects which are not finagle and there is no established "finagle model" Adding a hook like "timestamp but not for finagle" to a finagle project sounds like spahagetti to me. On 15 Mar 2017 08:23, "Naoman Abbas" <[email protected]> wrote: I think we should keep this in the same adjuster class (FinagleAdjuster) for following two reasons: 1. We are making an adjustment for Finagle Span model <openzipkin/zipkin-finagle#10> shortcomings, so anyone who was using Finagle Span model for library instrumentation will need this. 2. The adjustment is exactly same as the "apply-timestamp-and-duration" adjustment, just the condition to apply is different. So its better to reuse the code, instead of having multiple copies. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#36 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAD6170_ouPg-lWWaTbRfN-poBkJKI0hks5rl4PNgaJpZM4Mb7hc> .

[naoman]

While lack of timestamp and duration is not finagle centric, it is an issue with finagle. Even before this change, we were adjusting timestamp and duration for spans coming form finagle libraries. With this change, all we are doing is extending it to the spans that are created based on finagle span model. Finagle is the common factor here.

I understand that other libraries can also have this issue, but lets tackle that when we run into them. Maybe they need a different handling than this. In case of finagle, we know that the "adjustment" is exactly same in both cases (spans coming from finagle library, and spans coming based on finagle model).

I've updated the property name to make it explicit.

[codefromthecrypt]

I don't agree at all with this change. finagleSpanModel is jargon I've not heard of before.

openzipkin/openzipkin.github.io#49 has details on many libraries which all need to accomodate timestamp and duration. This is a well investigated topic, it isn't at all hypothetical.

On the other hand the "non-finagle" libraries you are hinting at must not be open source, which hints to me that using a custom adjuster is probably the way out. Finagle always adds finagle version properties and the point of this module is to address the finagle library.

[codefromthecrypt]

here's a summary of the approach I mentioned for how to address libraries that simply don't report timestamp or duration (regardless of how finagly) #37