[improvement] Refactor Tracer.initTrace to take enum instead of optional boolean #166
Conversation
|
This is using the names @iamdanfox initially wrote in #163 (comment). I think these look clear enough, but let me know if you come up with better names for this. |
yixunx
force-pushed
the
yx/refactor-optional-boolean
branch
from
d319c94 to
434627e
Compare
| return Observability.SAMPLER_DECIDES; | ||
| } else { | ||
| return Optional.of(header.equals("1")); | ||
| return header.equals("1") ? Observability.SAMPLE : Observability.DO_NOT_SAMPLE; |
There was a problem hiding this comment.
Happy this no longer allocates 😄
| return Observability.SAMPLER_DECIDES; | ||
| } else { | ||
| // No need to box the resulting boolean and allocate | ||
| // a new Optional wrapper for each invocation. |
There was a problem hiding this comment.
Can probably remove this comment
yixunx
changed the title
yixunx
force-pushed
the
yx/refactor-optional-boolean
branch
from
67b283d to
9f3b64b
Compare
| public enum Observability { | ||
| SAMPLE, | ||
| DO_NOT_SAMPLE, | ||
| SAMPLER_DECIDES |
|
Think I've addressed all existing comments, and this is good for another review |
| SAMPLE, | ||
| DO_NOT_SAMPLE, | ||
| UNDECIDED | ||
| } |
There was a problem hiding this comment.
Mind adding docs to this class?
Big fan of this :-)
carterkozak
left a comment
carterkozak
left a comment
There was a problem hiding this comment.
Generally lgtm, thanks for the deprecated backcompat methods.
| private static Optional<Boolean> hasSampledHeader(ContainerRequestContext context) { | ||
| private static Observability getObservabilityFromHeader(ContainerRequestContext context) { | ||
| String header = context.getHeaderString(TraceHttpHeaders.IS_SAMPLED); | ||
| if (header == null) { |
There was a problem hiding this comment.
It's a slight behavior change, but we may want Strings.isNullOrEmpty(header) here
There was a problem hiding this comment.
In favor of this, would lead to less potential confusion down the line.
| return Observability.UNDECIDED; | ||
| } else { | ||
| return Optional.of(header.equals("1")); | ||
| return header.equals("1") ? Observability.SAMPLE : Observability.DO_NOT_SAMPLE; |
There was a problem hiding this comment.
pedantic nit: would prefer "1".equals(header), though it won't make a difference in practice here since we've already validated non-null
5 participants
Before this PR
Tracer.initTracetakes a slightly awkward approach to use an optional boolean to represent a tri-state (relevant discussion at #163 (comment))After this PR
Added a new version of
Tracer.initTracethat takes an enum instead of an optional boolean for the observability parameter. The old method that takes an optional is deprecated.