Use X-Ray propagator for aws sdk 1.1 instrumentation #2117
Conversation
anuraaga
requested review from
iNikem,
jkwatson,
mateuszrzeszutek,
pavolloffay,
trask and
tylerbenson
as code owners
| @@ -80,8 +80,7 @@ public Context startSpan( | |||
| private void inject(Context context, CARRIER carrier) { | |||
| Setter<CARRIER> setter = getSetter(); | |||
| if (setter == null) { | |||
| throw new IllegalStateException( | |||
There was a problem hiding this comment.
@trask I think this stopped making sense when making all the startSpan return Context since there's no way to skip injection otherwise.
There was a problem hiding this comment.
other options, just in case you like either of them better:
- make this
injectmethod protected so subclasses can override - create a separate (protected)
getTextMapPropagator()method that returns the global, but can be overridden to return something else
though both of these options do increase the (protected) API surface
There was a problem hiding this comment.
protected inject seems nice yeah
| @@ -80,8 +80,7 @@ public Context startSpan( | |||
| private void inject(Context context, CARRIER carrier) { | |||
| Setter<CARRIER> setter = getSetter(); | |||
| if (setter == null) { | |||
| throw new IllegalStateException( | |||
There was a problem hiding this comment.
other options, just in case you like either of them better:
- make this
injectmethod protected so subclasses can override - create a separate (protected)
getTextMapPropagator()method that returns the global, but can be overridden to return something else
though both of these options do increase the (protected) API surface
…nstrumentation into aws-sdk-11-xray
|
I am a little bit worried about this magic. I mean, we/user configure propagator and it works, but all of a sudden some instrumentations use another propagator. This may be very surprising. We at least have to document this loud and clear. |
|
@iNikem Oops yeah - copied over disclaimer from v2 README |
@kubawach I know this overlaps a bit with yours, sorry! @pavolloffay asked me to look into an issue with request signing.
@pavolloffay I couldn't reproduce an issue here unfortunately - I was expecting the
server.lastRequest.headers.get("traceparent") == nullassertion to fail without a separate change but the apache instrumentation seems to not be messing with the signature.