Add builders for setting optional attributes on HTTP extractors #5347
Conversation
| HttpClientAttributesExtractor.builder(new TestHttpClientAttributesGetter()) | ||
| .captureHttpHeaders( | ||
| CapturedHttpHeaders.create( | ||
| singletonList("Custom-Request-Header"), | ||
| singletonList("Custom-Response-Header"))) | ||
| .build(); |
There was a problem hiding this comment.
what do you think of hiding CapturedHttpHeaders and exposing captureHttpRequestHeaders and captureHttpResponseHeaders on the builders?
There was a problem hiding this comment.
I think I'd probably prefer leaving this as it is, it's a bit simpler this way.
If I added captureHttpRequestHeaders(List<String>) I'd have to take the following things into account:
- is it additive? does
. captureHttpRequestHeaders(["abc"]).captureHttpRequestHeaders(["def"])capture["abc", "def"]or does the second call overwrite the first one? - do the default (configured) values get overwritten, or appended to?
With captureHttpHeaders(CapturedHttpHeaders) it's clear (I hope so? 😅 ) that the previous setting is overwritten. (Maybe I should rename it to setCapturedHttpHeaders after all...)
There was a problem hiding this comment.
How about
setCapturedRequestHeaders
setCapturedResponseHeaders
There was a problem hiding this comment.
I'll do that in a separate PR - this requires changing all our HTTP library instrumentations, so the amount of changes will be pretty significant.
mateuszrzeszutek
force-pushed
the
http-attribute-extractor-builders
branch
from
583e894
to
a44bcbf
Compare
…-telemetry#5347) * Add builders for setting optional attributes on HTTP extractors * errorprone * fix compilation failure
No description provided.