Implement mechanism to apply custom api path #1859
Conversation
…eUrl. This comes in handy when another monitoring tool is being used, for instance new relic, which doesn't not require path in its tracing api.
There was a problem hiding this comment.
LGTM apart from small changes. Could you also please update the documentation and mention the new apiPath somewhere around here https://docs.spring.io/spring-cloud-sleuth/docs/2.2.7.RELEASE/reference/html/#sending-spans-to-zipkin ? Once it's done then @Buzzardo would review it.
| @@ -77,6 +81,16 @@ else if (this.encoding == Encoding.JSON) { | |||
| this.messageEncoder = BytesMessageEncoder.forEncoding(this.encoding); | |||
| } | |||
|
|
|||
| private String buildUrlWithCustomPathIfNecessary(final String baseUrl, | |||
| final String customApiPath, final String defaultUrl) { | |||
| if (Objects.nonNull(customApiPath)) { | |||
There was a problem hiding this comment.
Actually we could call StringUtils.hasText(...)
There was a problem hiding this comment.
@marcingrzejszczak , using nonNull was the intention. This is to cater cases where:
- Null (when the apiPath is not configured at all, use the default
- NonBlank (any text) as the custom api path
- Blank (empty string) this is for cases no path needed at all (one example newrelic)
There was a problem hiding this comment.
apiPathshould default toapi/v2/spans- that way we're simplifying the cases.- Makes sense
- Shouldn't it be then
/?
There was a problem hiding this comment.
@marcingrzejszczak
We can't default to api/v2/spans in the apiPath because within RestTemplate the path is different between JSON2, PROTO and JSON. JSON is using v1. So I wanted to ensure that part of logic within the RestTemplateSender constructor being maintained.
For 3, at least for new relic case, there's no /. If you include /, tracing api will return 404. So for the path it must be empty. This approach also give flexibility to include / or not.
There was a problem hiding this comment.
OK, now I understand! You're right 👍
| @@ -84,6 +91,14 @@ public void setBaseUrl(String baseUrl) { | |||
| this.baseUrl = baseUrl; | |||
| } | |||
|
|
|||
| public String getApiPath() { | |||
| return apiPath; | |||
There was a problem hiding this comment.
return this.apiPath - let's be consistent
There was a problem hiding this comment.
will include in the next push. Thanks @marcingrzejszczak
| @@ -37,6 +37,13 @@ | |||
| */ | |||
| private String baseUrl = "http://localhost:9411/"; | |||
|
|
|||
| /** | |||
There was a problem hiding this comment.
cc @Buzzardo - can you please check this out?
| @@ -37,6 +37,13 @@ | |||
| */ | |||
| private String baseUrl = "http://localhost:9411/"; | |||
|
|
|||
| /** | |||
| * Api path that will be appended to baseUrl above as suffix This is applicable if you | |||
There was a problem hiding this comment.
Change to:
The API path to append to the baseUrl (above) as a suffix. This applies if you
| @@ -37,6 +37,13 @@ | |||
| */ | |||
| private String baseUrl = "http://localhost:9411/"; | |||
|
|
|||
| /** | |||
| * Api path that will be appended to baseUrl above as suffix This is applicable if you | |||
| * are using other monitoring tools. IE, new relic, the trace api doesn't need api | |||
There was a problem hiding this comment.
Change to:
use other monitoring tools, such as New Relic. The trace API doesn't need the API
| /** | ||
| * Api path that will be appended to baseUrl above as suffix This is applicable if you | ||
| * are using other monitoring tools. IE, new relic, the trace api doesn't need api | ||
| * path, so this can be set to blank ("") in the config. |
There was a problem hiding this comment.
Change to:
path, so you can set it to blank ("") in the configuration.
...zipkin/src/main/java/org/springframework/cloud/sleuth/zipkin2/sender/RestTemplateSender.java
Show resolved
Hide resolved
|
@marcingrzejszczak @Buzzardo made the changes as well in the main documentation de23b6e . Thanks. |
|
Thanks @MazizEsa for your contribution! |
This comes in handy when another monitoring tool is being used, for instance new relic, which doesn't not require path in its tracing api.
Context as of feb 19, new relic doesn't accept request to its tracing api endpoint with path. Previous it was fine, now it will throw 404 erro.