Fallback to db.connection_string for sql backend url #84
Conversation
| if (DbSemanticConventionUtils.isSqlTypeBackendForOtelFormat(attributeFieldMap)) { | ||
| Optional<String> sqlUrl = DbSemanticConventionUtils.getBackendURIForOtelFormat(attributeFieldMap); | ||
| sqlUrl.ifPresent(s -> eventBuilder.getSqlBuilder().setUrl(s)); | ||
| if (sqlUrl.isPresent()) { |
There was a problem hiding this comment.
Can't we use getSqlURI form DbSemanticsConvention?
There was a problem hiding this comment.
That accepts Event object, we have an attribute map here. I can create one more method there. Should I?
There was a problem hiding this comment.
Yes, it's a common code, and it's difficult to maintain.
|
Some unit tests? |
Codecov Report
@@ Coverage Diff @@
## main #84 +/- ##
============================================
+ Coverage 68.68% 68.99% +0.31%
Complexity 814 814
============================================
Files 83 83
Lines 3442 3441 -1
Branches 367 367
============================================
+ Hits 2364 2374 +10
+ Misses 933 922 -11
Partials 145 145
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
findingrish
changed the title
|
@rish691 snyk scan is failing. |
| } | ||
| if (SpanAttributeUtils.containsAttributeKey( | ||
| event, OTelDbSemanticConventions.DB_CONNECTION_STRING.getValue())) { | ||
| String url = SpanAttributeUtils.getStringAttribute( |
There was a problem hiding this comment.
Don't we have to convert this String to URI object?
try {
new URI(URL)
} catch (Mulformed) {
}
I guess Buchi fixed it here: #87 (comment) |
| event, OTelDbSemanticConventions.DB_CONNECTION_STRING.getValue())) { | ||
| String url = SpanAttributeUtils.getStringAttribute( | ||
| event, OTelDbSemanticConventions.DB_CONNECTION_STRING.getValue()); | ||
| if (!isValidURI(url)) { |
There was a problem hiding this comment.
nit, if you do isValirURI(url) { return Optional.of(url); } I think you can just keep the last return Optional.empty().
There was a problem hiding this comment.
Sure, I will do it in a subsequent pr.
| } | ||
|
|
||
| @Test | ||
| public void testGetSqlUrlForOtelFormat() { |
There was a problem hiding this comment.
I wonder if we need some test where the user/password are included in the URL. Not sure tif that is a thing among jdbc.
There was a problem hiding this comment.
You mean if username/pwd is present we should drop the url?
|
Late to the party @rish691 but left you a couple of comments. |
No description provided.