-
Notifications
You must be signed in to change notification settings - Fork 37
add support oracle tns parser for DbConnDetails #1304
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
Hi @dantedenis, |
|
Gentle ping |
|
Hi @dantedenis, could you please add an example application in the /example folder of the project? It would help us validate the changes with a sample app. Also, please include any required docker-compose or podman-compose files in the same folder. Thanks. |
|
may be best to add case like db2 if isDB2driver(driverName) {
if details, ok := parseDB2ConnDetailsKV(connStr); ok {
return details
}
return DbConnDetails{RawString: connStr}
}add isOracledriver(driverName) {
if details, ok := parseOracleConnDetailsTNS(connStr); ok {
return details
}
return DbConnDetails{RawString: connStr}
}and parse oracle connection string or string like |
|
@vtolstov: Just wanted to understand your requirement clearly. As you mentioned DB2, for DB2, the spans being generated are generic SQL DB spans. If you want the same behaviour for Oracle, then I don’t think this Oracle-specific parsing code is required. Instana Go Tracer supports any database library built on Go’s database/sql. So, if the Oracle library you are using is based on database/sql, it will automatically generate a common DB span (similar to what we see for DB2). If you need Oracle specific parsing (like what we currently have for PostgreSQL or MySQL), then this PR alone will not be sufficient. The backend and UI also need changes to support Oracle-specific spans. That would be a new feature, and we will have to discuss it with our product team. For generic database spans, the connection string is passed using DbConnDetails{RawString: connStr}. This RawString is displayed in the UI, and if it contains a password, it will be visible in the Instana UI. If your concern is only about the password exposure (which applies to any generic DB, not just Oracle), we will provide a common method to mask the password. You will still be able to see Oracle DB spans under the generic SQL span structure with the relevant details. Just let me know your thoughts. |
866047e to
4c965ea
Compare
|
Hi @nithinputhenveettil, i added example |
Thanks @dantedenis. We’ll review it and get back to you as soon as possible. |
|
@dantedenis: I tested the changes using the example app you created. Screenshot 1 - with your change. Screenshot 2 - without your change. As mentioned earlier, even with your PR, the span created is still a generic SQL span, not an “oracle” span. I also tested the app without your change, and the UI still shows the same generic DB spans. The only differences are that the password is not masked in the connection string, and the host and port are not shown as separate fields, because our generic DB spans don’t have dedicated host/port fields, but this information is still available through the connection string. I have attached both screenshots, first with your PR, second without it. The database name you set (“oracle”) doesn’t appear because the backend/UI don’t have Oracle specific parsing support. Adding that would require a product feature request through Aha and further discussion with the product team. So, to summarise:
The one real issue visible in the screenshot is the password in the connection string. This is a common problem for all generic DB spans, not just Oracle. We’re working on a common solution to mask passwords across all generic DB spans, and we will provide a release with that enhancement. If you’re okay with this approach, we can decide not to merge this PR and instead proceed with a new release that adds password masking for generic DB spans. |
|
@nithinputhenveettil yes, the problem is that the user interface displays a password in span (view UI). And I added this just so that there would be no problems with its display, and this is exactly what you showed in the first screenshot, which is what we expected. |
|
when you want to provide release with generic masking feature for db spans ? |
|
@vtolstov : This feature is our top priority at the moment, and we’re actively working on it. We’ll share the release timeline as soon as it’s finalised. |


9 We may need a parser for OracleTNS. Should we add this?
And it's not very safe to just save the URI in Raw, because it may contain secrets.
instrumentation_sql.go 382: return DbConnDetails{RawString: connStr}It might be worth adding an optional parameter for custom escaping.
Alternatively, just add a global secret replacer that I can override in the project, and the tool will automatically check for its presence and execute the required action if it exists.
Not explicitly, but we can control it.
Thx 😊