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

Implement C3P0 connection pool metrics #6174

Merged
merged 5 commits into from
Jun 20, 2022

Conversation

agoallikmaa
Copy link
Contributor

No description provided.

@agoallikmaa agoallikmaa requested a review from a team June 14, 2022 13:04
try {
return supplier.getAsInt();
} catch (SQLException e) {
return 0;
Copy link
Member

Choose a reason for hiding this comment

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

@jack-berg any thoughts on recording 0 vs not recording any value in this case?

Copy link
Member

Choose a reason for hiding this comment

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

I'm thinking it may be better to re-throw an unchecked exception here instead of recording 0.

It looks like it will get caught and logged here: https://github.com/open-telemetry/opentelemetry-java/blob/f280f278be0222f24b7ad7a8dc7ffc293358785b/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/internal/state/CallbackRegistration.java#L100-L103

Or if we think this is common and don't want to log, we could consider suppressing it here:

Copy link
Member

Choose a reason for hiding this comment

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

Probably don't want instrumentation to be in the habit of throwing exceptions, even if the SDK can handle it. Better to have the handling to be explicit in the callback.

I do think that recording nothing is more accurate than 0.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The problem is that the only ways to record nothing with the current implementation of DbConnectionPoolMetrics would be to either throw an exception, or change DbConnectionPoolMetrics to use providers that return a Long instead that can be null. I changed it throw since I didn't see the last comment yet, but if you can suggest a better way, I can change it.

Copy link
Member

Choose a reason for hiding this comment

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

Do you expect exception to be thrown here on a regular basis? If so, it is likely to make user logs noisy. For reference, here's the code that handles these exceptions.

Copy link
Member

Choose a reason for hiding this comment

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

I looked briefly at the c3p0 implementation, and I couldn't really find any case where these exceptions would really be thrown, so I think it's probably ok

@laurit laurit merged commit ba912bc into open-telemetry:main Jun 20, 2022
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