Setup TLS Client for Zipkin #5085
Conversation
mmorel-35
force-pushed
the
ssl-zipkin
branch
2 times, most recently
from
93e1c73 to
8947559
Compare
Codecov ReportBase: 91.06% // Head: 90.84% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #5085 +/- ##
============================================
- Coverage 91.06% 90.84% -0.22%
- Complexity 4888 4890 +2
============================================
Files 553 553
Lines 14453 14513 +60
Branches 1381 1395 +14
============================================
+ Hits 13161 13184 +23
- Misses 895 926 +31
- Partials 397 403 +6
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
|
Are these properties in the specification? If not, we'll need to hold off until they are. |
|
Hi @jkwatson , How can I check that ? |
|
The environment variables for zipkin are defined in the spec here. |
So, these new ones proposed here are not in the specification. Before we can introduce new env vars, we'd really need them to be in the spec, unfortunately. The best way to get them there is to open up an issue with the specification and draft a PR to add the ones you need/want. However, I think (@jack-berg can confirm) that there's a current moratorium on adding any new environment variables until the configuration working-group has figured out a better long-term configuration solution. |
|
Yes there is currently a moratorium in place on adding new environment variables, as described here. |
|
These variables are here to provide client securisation calling on Zipkin collector endpoint the same way it is done on OTPL collector, I see them being under the publicated restrictions. Let's hope it is going to be accepted |
| new RetryInterceptor( | ||
| retryPolicy, | ||
| (response) -> | ||
| RetryUtil.retryableHttpResponseCodes().contains(response.code()))); |
There was a problem hiding this comment.
Is retry valid for zipkin? Is there some sort of specification of which status codes are retryable for zipkin? The status codes in RetryUtil.retryableHttpStatusResposneCodes() are defined in the OTLP specification.
There was a problem hiding this comment.
Why would it be different between the two ? Both are HTTP status code.
I'm way more interested by the securisation part , so if the retry is creating troubles I don't mind excluding it from this PR and keep it for another time
There was a problem hiding this comment.
Why would it be different between the two ? Both are HTTP status code
Maybe zipkin uses those status codes differently and returns one in situations where a retry would not solve the problem.
There was a problem hiding this comment.
I removed the retry part, so I keep this PR focused only on TLS client.
Signed-off-by: Matthieu MOREL <matthieu.morel35@gmail.com>
|
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
|
Closed as inactive. Feel free to reopen if this PR is still being worked on. |
It is not actually possible to configure client TLS for Zipkin.
This add the following options :
This is highly inspired by the way it is done for Otlp Client
I'll add the coverage tests before making it ready to review.
Signed-off-by: Matthieu MOREL matthieu.morel35@gmail.com