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

Add instrumentation for druid connection pool #9935

Merged
merged 19 commits into from
Dec 21, 2023

Conversation

AlchemyDing
Copy link
Member

resolve issue
connection pools mertric, like hikaricp, DBCP ,c3p0 , etc...

@AlchemyDing AlchemyDing requested a review from a team as a code owner November 22, 2023 13:53
@AlchemyDing AlchemyDing changed the title feat:instrument alibaba/druid DB connection pool Add instrumentation for druid connection pool Nov 23, 2023
}

dependencies {
library("com.alibaba:druid:0.2.6")
Copy link
Member

Choose a reason for hiding this comment

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

For simplicity, and since 1.0.0 was released in 2013 I'd recommend going ahead and baselining against 1.0 and renaming the module to druid-1.0

Copy link
Member Author

Choose a reason for hiding this comment

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

@trask done, thank you

group.set("com.alibaba")
module.set("druid")
versions.set("[0.2.6,)")
skip("1.0.30")
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
skip("1.0.30")
skip("1.0.30")
assertInverse.set(true)

Copy link
Member Author

Choose a reason for hiding this comment

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

@trask assertInverse.set(true) will make muzzle3 check failed ,history check

Copy link
Contributor

Choose a reason for hiding this comment

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

I think your best option is to set the version range back to versions.set("[0.2.6,)") so you could add assertInverse.set(true). The same approach is used in

Alternatively if there is a class that appears in 1.0 but does not appear in earlier versions you could use something like
public ElementMatcher.Junction<ClassLoader> classLoaderMatcher() {
return hasClassesNamed("javax.servlet.Servlet");
}
to limit the range where the instrumentation works.

Copy link
Member Author

Choose a reason for hiding this comment

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

@laurit Early versions may not have the DruidDataSourceStatManager.addDataSource method, or the parameters may be different, such as
0.2
0.2.5

Copy link
Member Author

Choose a reason for hiding this comment

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

Can I write in this way

muzzle {
  pass {
    group.set("com.alibaba")
    module.set("druid")
    versions.set("[0.1.18,)")
    skip("1.0.30")
    assertInverse.set(true)
  }
}

Copy link
Contributor

Choose a reason for hiding this comment

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

@AlchemyDing sure, I think you can also use versions.set("(,)") if muzzle passes on all published versions.

Copy link
Member Author

Choose a reason for hiding this comment

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

@laurit done, thank you

@laurit
Copy link
Contributor

laurit commented Dec 8, 2023

Overall looks nice, the only open question for me is whether we should name this alibaba-druid instead of druid because there is also https://druid.apache.org/ which is a completely different thing. @trask @mateuszrzeszutek wdyt?

@AlchemyDing
Copy link
Member Author

Overall looks nice, the only open question for me is whether we should name this alibaba-druid instead of druid because there is also https://druid.apache.org/ which is a completely different thing. @trask @mateuszrzeszutek wdyt?

I think named alibaba-druid is better.

@trask trask enabled auto-merge (squash) December 21, 2023 16:38
@trask trask merged commit 20ab012 into open-telemetry:main Dec 21, 2023
47 checks passed
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.

Discuss feature idea of instrument alibaba/druid DB connection pool
3 participants