Skip to content

Add instrumentation for druid connection pool#9935

Merged
trask merged 19 commits into
open-telemetry:mainfrom
AlchemyDing:feature-druid
Dec 21, 2023
Merged

Add instrumentation for druid connection pool#9935
trask merged 19 commits into
open-telemetry:mainfrom
AlchemyDing:feature-druid

Conversation

@AlchemyDing

Copy link
Copy Markdown
Member

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

@AlchemyDing
AlchemyDing requested a review from a team 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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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

@AlchemyDing
AlchemyDing requested a review from trask December 6, 2023 14:32
@laurit

laurit commented Dec 8, 2023

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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.

@AlchemyDing
AlchemyDing requested a review from laurit December 8, 2023 13:42
@AlchemyDing
AlchemyDing requested a review from laurit December 8, 2023 15:29
@trask
trask enabled auto-merge (squash) December 21, 2023 16:38
@trask
trask merged commit 20ab012 into open-telemetry:main Dec 21, 2023
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