adding support for null path in generic_url_like

[cmunger]

Some unsupported jdbc driver using JdbcConnectionUrlParser.GENERIC_URL_LIKE may return a null path creating a nullpointer exception and not setting correctly the uri host and port

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: cmunger (32a5bce)

[mateuszrzeszutek]

Thanks!
Can you also add a test case that covers this scenario?

[cmunger]

Well i tried to add a testcase but had really hard time with the build and setting up groovy in my IDE, after some time i gave up.. let me know if the testcase is mandatory for this small change

[trask]

@cmunger can you give an example of a jdbc url that will throw NullPointerException here? I can plug add it as a test case in JdbcConnectionUrlParserTest if you are still having trouble.

[cmunger]

Hello trask thanks for your proposal as i've really struggled to setup the dev env :

[otel.javaagent 2022-11-09 09:13:29:864 +0100] [main] DEBUG io.opentelemetry.javaagent.shaded.instrumentation.jdbc.internal.JdbcConnectionUrlParser - null
java.lang.NullPointerException
	at io.opentelemetry.javaagent.shaded.instrumentation.jdbc.internal.JdbcConnectionUrlParser$1.doParse(JdbcConnectionUrlParser.java:52)
	at io.opentelemetry.javaagent.shaded.instrumentation.jdbc.internal.JdbcConnectionUrlParser.parse(JdbcConnectionUrlParser.java:874)
	at zzz.zzzzzzzzz.zzz.zzz.ProxyDriver.connect(ProxyDriver.java:xxx)
	at org.apache.tomcat.jdbc.pool.PooledConnection.connectUsingDriver(PooledConnection.java:319)
	at org.apache.tomcat.jdbc.pool.PooledConnection.connect(PooledConnection.java:212)
	at org.apache.tomcat.jdbc.pool.ConnectionPool.createConnection(ConnectionPool.java:744)

we are using a proxy driver that was used for metrics and we will use this kind of URL :

jdbc:proxy:jdbc:oracle:thin:@oracle-xe.docker:1521:XE

so the important point is that it uses a db driver that is not stock supported by opentelemetry (jdbc:proxy) and will end up using JdbcConnectionUrlParser.GENERIC_URL_LIKE connection parses where the NPE occurs
hope it helps

[trask]

@cmunger I wasn't able to push to your branch, but you can cherry-pick the commit from here: trask@d2c6399

though you'll notice that even after removing the NPE, it still doesn't pick up the host/port

[cmunger]

thanks @trask , i took your test and i realized that the fix for the null path is kind indeed useless and does not provides correct DBInfo for this specific case with a proxy jdbc driver, i modified as you can see the codebase and test to correctly parse the jdbc url by looking at the last index of jdbc:, all the tests with other urls where still green and it should not create side effects i think, i let your team decide if they want to take this or not. Thanks for your help

dismissing stale review

[trask]

hey @cmunger, what is jdbc:proxy:...? maybe it makes sense to create special handling for that (similar to how other jdbc prefixes are handled)?

[cmunger]

@trask this jdbc:proxy prefix is an enterprise custom made proxy driver that is used for stats and monitoring purposes over other existing jdbc drivers (like jdbc:proxy:jdbc:oracle.....), we are migrating to your stack but all app cannot be upgraded quickly to stop using this proxy driver. As this is is not a generic tool with open source code i don't think it's appropriate to create a special handling like for other jdbc drivers. With this fix the target driver connection info are correctly handled and should not create regressions in handling of other drivers db info parsing.

[mateuszrzeszutek]

As this is is not a generic tool with open source code i don't think it's appropriate to create a special handling like for other jdbc drivers.

Consider excluding your custom proxy driver classes/package from instrumentation: https://github.com/open-telemetry/opentelemetry-java-instrumentation/blob/main/docs/advanced-configuration-options.md#excluding-specific-classes-from-being-instrumented

Still, even if that works for you, I do believe that we should include the if (path == null) part - throwing an NPE is a no-go, I think it's better (less bad? 😅 ) to have broken telemetry than a broken application.

(also linking related issue #5012)

[trask]

@cmunger the current fix seems targeted at there being a second jdbc:, which doesn't seem like a typical pattern for wrappers, so not sure if we should encode that logic.

I really like @mateuszrzeszutek's suggestion, can you see if you can get that to work?

[cmunger]

@mateuszrzeszutek i'll try your class exclusions setting hint, i m just worried it will stop instrumentalizing then underlying driver.. concerning the if (path == null) missing it will not crash the opentelementry app, simply will not parse correctly the dbinfo and will be missing info when data is exported from jvm to kibana apm or other system.. i guess you can close this pr then. thanks for your help