Review suggestions #1
Conversation
Improvements - MethodHandles are not @nullable - Both getFirstHeader and getAllHeaders method handles are used in HeadersExtractorBridge
Allows for internal iteration which can be more efficient - No forced Iterator allocations - No conversion from Header to String/byte[] necessary - Consumer is stateless and can be re-used, due to state method argument
eyalkoren
left a comment
eyalkoren
left a comment
There was a problem hiding this comment.
Thanks for the suggestions!
The generics makes the APIs slightly more verbal for use, but I admit it's nicer they can enforce the relationships between the carrier and the HeaderGetter using it.
I also like the change of the HeaderGetter API for multiple header values- it indeed makes implementations simpler.
|
|
||
| public abstract class AbstractHeaderGetter<T, C> implements HeaderGetter<T, C> { | ||
| @Override | ||
| public <S> void forEach(String headerName, C carrier, S state, HeaderConsumer<T, S> consumer) { |
There was a problem hiding this comment.
Should we encourage hiding from the agent when multiple header values iteration is not supported?
| transaction = tracer.startChildTransaction(headersExtractor, headersExtractorBridge, clazz.getClassLoader()); | ||
| if (headersExtractor != null) { | ||
| HeadersExtractorBridge headersExtractorBridge = HeadersExtractorBridge.get(getFirstHeader, getAllHeaders); | ||
| transaction = tracer.startChildTransaction(new HeadersExtractorBridge.Extractor(headerExtractor, headersExtractor), headersExtractorBridge, clazz.getClassLoader()); |
There was a problem hiding this comment.
Why not reusing the same instance? See how I changed in the pushed code, let me know if you see a problem with that.
| <S> void forEach(String headerName, C carrier, S state, HeaderConsumer<T, S> consumer); | ||
|
|
||
| interface HeaderConsumer<T, S> { | ||
| void accept(T headerValue, S state); |
There was a problem hiding this comment.
The consumer states I currently see are:
- before header iteration
- during header iteration
- after header iteration
Since the caller (i.e. agent) is aware of these states and can easily make the consumer aware of them, I currently don't see a user for the state. What did you have in mind? Can you provide an example?
There was a problem hiding this comment.
OK, implementing now and realized what you had in mind 👍
…rics Suggested changes for cgroup metrics
2 participants
Review suggestions for elastic#1024