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 HikariCP connection pool metrics #6003

Merged
merged 4 commits into from
May 13, 2022

Conversation

mateuszrzeszutek
Copy link
Member

Resolves #5860

@mateuszrzeszutek mateuszrzeszutek requested a review from a team May 10, 2022 13:49

package io.opentelemetry.javaagent.instrumentation.hikaricp;

import com.zaxxer.hikari.metrics.IMetricsTracker;
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice - let's go ahead and add library instrumentation

Copy link
Member Author

Choose a reason for hiding this comment

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

Do you mind if I do that in a separate PR? This one is not that small already, and it establishes some common code parts that might be used in other connection pill instrumentations.

Copy link
Contributor

Choose a reason for hiding this comment

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

What if the library instrumentation was contributed to Hikari iteself and lived alongside the dropwizard, micrometer, and prometheus implementations?

Copy link
Member Author

Choose a reason for hiding this comment

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

That could also work, probably.
The semantic conventions are not stable though, they could change in the future -- I wonder what kind of strategy we have for updating 3rd party libs when the telemetry changes.

Copy link
Contributor

Choose a reason for hiding this comment

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

Splitting library out in separate PR is fine too - that being said "establishes some common code parts that might be used in other connection pill instrumentations" makes it sound like other instrumentation might come before a split, which I wouldn't recommend ;)

The semantic conventions are not stable though, they could change in the future -- I wonder what kind of strategy we have for updating 3rd party libs when the telemetry changes.

Most stable libraries will be reluctant if not forbid having a dependency on an alpha module, our -semconv module. I would probably keep upstream instrumentation off the table until the semconv is stable and part of our non-alpha artifact.

Copy link
Contributor

@breedx-splk breedx-splk left a comment

Choose a reason for hiding this comment

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

Looks good to me. I did make a couple observational comments tho.


package io.opentelemetry.javaagent.instrumentation.hikaricp;

import com.zaxxer.hikari.metrics.IMetricsTracker;
Copy link
Contributor

Choose a reason for hiding this comment

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

What if the library instrumentation was contributed to Hikari iteself and lived alongside the dropwizard, micrometer, and prometheus implementations?

}

@Test
void shouldNotBreakCustomUserMetrics() throws Exception {
Copy link
Contributor

Choose a reason for hiding this comment

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

👍🏻 good to cover this!

point.hasAttributes(
Attributes.of(stringKey("pool.name"), poolName))))));
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Favorite file formatting all week, LOL! 🙈

Copy link
Member Author

Choose a reason for hiding this comment

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

This is the price we have to pay for not using Groovy 😂

Copy link
Contributor

Choose a reason for hiding this comment

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

And what a price it is!

Copy link
Member

Choose a reason for hiding this comment

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

😂

@trask trask added this to the 1.14.0 milestone May 12, 2022
point.hasAttributes(
Attributes.of(stringKey("pool.name"), poolName))))));
}
}
Copy link
Member

Choose a reason for hiding this comment

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

😂

Comment on lines +13 to +23
@AutoService(IgnoredTypesConfigurer.class)
public class IgnoredTestTypesConfigurer implements IgnoredTypesConfigurer {

@Override
public void configure(Config config, IgnoredTypesBuilder builder) {
// we don't want to instrument auto-generated mocks
builder
.ignoreClass("org.mockito")
.ignoreClass("com.zaxxer.hikari.metrics.IMetricsTracker$MockitoMock$");
}
}
Copy link
Member

Choose a reason for hiding this comment

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

I think it's reasonable for this to be in the hikaricp module (non-testing artifact), especially since it should affect perf due to the underlying trie (as opposed to the old loop), and users may want to run their own tests using the javaagent.

(I'm also ok with it here)

Comment on lines +28 to +29
// this method is always called in the HikariPool constructor, even if the user does not
// configure anything
Copy link
Member

Choose a reason for hiding this comment

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

👍

@trask trask merged commit 5bcab32 into open-telemetry:main May 13, 2022
@mateuszrzeszutek mateuszrzeszutek deleted the hikari branch May 13, 2022 05:43
RashmiRam pushed a commit to RashmiRam/opentelemetry-auto-instr-java that referenced this pull request May 23, 2022
* Implement HikariCP connection pool metrics

* rebase after SDK update

* fix muzzle

* code review comments
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.

Adding Hikari Metrics
5 participants