[GitHub] [camel] kremers commented on a diff in pull request #8774: camel-splunk-hec - Add endpoint url suffix configuration to enable op…

[GitBox <gi...@apache.org>]

kremers commented on code in PR #8774:
URL: https://github.com/apache/camel/pull/8774#discussion_r1032265181


##########
components/camel-splunk-hec/src/main/java/org/apache/camel/component/splunkhec/SplunkHECConfiguration.java:
##########
@@ -99,6 +101,20 @@ public void setHost(String host) {
         this.host = host;
     }
 
+    /**
+     * Splunk endpoint
+     *   Defaults to /services/collector/event
+     *   To write RAW data like JSON use /services/collector/raw
+     *   To extract timestamps in Splunk>8.0 /services/collector/event?auto_extract_timestamp=true
+     */
+    public void setEndpoint(String endpoint) {

Review Comment:
   Hi Claus, thank you very much for your quick reply and check of the PR!
   
   - Thank you very mich for the hint with the name, okay to set it to "SplunkRESTEndpoint"? Which would be a better naming. 
   
   - If the url includes a question mark the user can utilize RAW{} https://camel.apache.org/manual/faq/how-do-i-configure-endpoints.html#HowdoIconfigureendpoints-Configuringparametervaluesusingrawvalues so it will work. Modules like azure-eventhub do make excessive use of it for connection strings, passwords, ... for example:
   
   `.from(azure-eventhubs:?connectionString=RAW({{camel.component.azure-eventhubs.connection-string}}) ...`
   
   - For the reason of extendability, enum does not sound like a good fit to me, since it requires changes to the software and different versions in place on every new Splunk version/endpoint. The ENUM may also suggest RESTEndpoints that are not available in older Splunk versions. So I suggest to stick with a strong default that does not introduce a breaking change, but full flexibility to adpot to new Splunk versions without changing the camel module. See: https://docs.splunk.com/Documentation/SplunkCloud/8.2.2203/Data/HECRESTendpoints
   
   - Naming in the module can be improved a lot, since hostAndIp are written aus SplunkURL where users normally expect an URL compliant to RFC1738 (https://datatracker.ietf.org/doc/html/rfc1738) including the REST endpoint including http or https (including RAW() for handling slashes). One possible way to improve and keep backwards compability would be introduction of a configuration parameter "connectionString" that has first priority if set and includes all information. But I guess that is better adressed in a second pull request.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org