Skip to content
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

Added JDBC tracing section in Java Observability #1278

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

davidhunglam
Copy link
Contributor

Initial draft about enabling JDBC tracing in the Hana JDBC driver for CAP Java applications.

@@ -218,6 +218,57 @@ In case you've configured `cf-java-logging-support` as described in [Logging Ser
By default, the ID is accepted and forwarded via HTTP header `X-CorrelationID`. If you want to accept `X-Correlation-Id` header in incoming requests alternatively,
follow the instructions given in the guide [Instrumenting Servlets](https://github.com/SAP/cf-java-logging-support/wiki/Instrumenting-Servlets#correlation-id).

### JDBC Tracing in SAP Hana
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is very complicated. I was expecting the configuration maybe also to be a little CAP-specific. These docs should actually be part of the HANA JDBC driver documentation from my perspective..

Could we try if the approach we discussed yesterday with data source properties works as well? That would be more CAP-specific (as you need to know where to place these properties in application.yaml) and also much simpler and platform independent.

@davidhunglam
Copy link
Contributor Author

davidhunglam commented Oct 2, 2024

Hi @renejeglinsky, as parts of this documentation are rather CAP-unspecific (especially tracing activation via command line), I'm not sure if this the right part of our Capire to add this information to.

As Marc said, this should actually be in the HANA JDBC driver documentation, but that documentation is lacking severely in details, and users will surely contact us for guidance.

What do you think, should it better be placed elsewhere?

@renejeglinsky
Copy link
Contributor

Hi @davidhunglam ,
if we can't link to docs that we deem appropriate, then it's fine to have the docs in capire. Especially in this case, where it's not too much.

Anyway we could provide feedback to the HANA colleagues about what we would expect. And if they can provide those docs, we can then remove it here and set just a link. Would you/Marc like to give feedback?

service-manager: # name of service binding
hikari:
data-source-properties:
traceFile: "/home/user/jdbctraces/trace_.log" # use a path that is write accessible
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does this only work with absolute paths? Or would jdbctraces/trave_.log would write to to jdbctraces in the root of the application / container?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Relative paths work as well. But the important thing is to use a directory the current user (in the cloud native buildpack that's cnb) has write access to. Otherwise nothing will be written silently, there'd be no error message.


[Trace Options](https://help.sap.com/docs/SAP_HANA_CLIENT/f1b440ded6144a54ada97ff95dac7adf/4033f8e603504c0faf305ab77627af03.html) lists available command line options. For the datasource property only use the option name, such as `CONNECTIONS`, `API` or `PACKET`. You can specify more than one option, separated by commas.

This method of activating JDBC tracing requires restarting the application. For cloud deployments on Cloud Foundry this typically means redeploying via MTA, on Kyma this means rebuilding the application, recreating and publishing the container image to the container image registry and redeploying the application via Helm.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if the JDBC trace configuration is something I wanted to persist in the application.yaml file. I would rather document the approach with environment variables and restaging the application, instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was not able to make it work with the environment variable only. Somehow the environment variable is only able to overwrite the existing entry from the application.yaml.

@davidhunglam
Copy link
Contributor Author

Hi @davidhunglam , if we can't link to docs that we deem appropriate, then it's fine to have the docs in capire. Especially in this case, where it's not too much.

Anyway we could provide feedback to the HANA colleagues about what we would expect. And if they can provide those docs, we can then remove it here and set just a link. Would you/Marc like to give feedback?

Hi @renejeglinsky, we've talked internally about it, and we've decided that our Capire might be the better place for the documentation. The Hana driver documentation is kept strictly environment agnostic, so they don't really care about where the driver runs.

@renejeglinsky
Copy link
Contributor

Hi @davidhunglam , if we can't link to docs that we deem appropriate, then it's fine to have the docs in capire. Especially in this case, where it's not too much.
Anyway we could provide feedback to the HANA colleagues about what we would expect. And if they can provide those docs, we can then remove it here and set just a link. Would you/Marc like to give feedback?

Hi @renejeglinsky, we've talked internally about it, and we've decided that our Capire might be the better place for the documentation. The Hana driver documentation is kept strictly environment agnostic, so they don't really care about where the driver runs.

Thanks! :)

@davidhunglam davidhunglam marked this pull request as ready for review November 6, 2024 09:44
Copy link
Contributor

@rjayasinghe rjayasinghe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can take this a first iteration.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants