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

Support tracing the ClickHouse Java HTTP client #11660

Merged
merged 8 commits into from
Jul 9, 2024

Conversation

jaydeluca
Copy link
Member

Contributes to #11562

Instruments v1 of the clickhouse-client (https://github.com/ClickHouse/clickhouse-java/tree/main/clickhouse-client)

A few notes:

  • I used version 0.5.0 as the earliest version of the clickhouse client in the instrumentation (most recent version is 0.6.1), if we think we need to go back further I can do that, but it felt like the client changes frequently, and with the release of the new client it didn't feel necessary.
  • In 0.6.1 they have introduced an alpha of a v2 client. Once that stabilizes I can followup and instrument that as well

The tests cover all the same scenarios, but I also created a repo to experiment with the tracing and visualize with jaeger and all seems to work as expected:
https://github.com/jaydeluca/clickhouse-tracing

image

Example Async query (execute)

async

Example error:

image

@jaydeluca jaydeluca requested a review from a team as a code owner June 23, 2024 15:12
@github-actions github-actions bot requested a review from theletterf June 23, 2024 15:13
docs/supported-libraries.md Outdated Show resolved Hide resolved
…ference to internal utility class, add call depth checking

@Override
public List<TypeInstrumentation> typeInstrumentations() {
return Collections.singletonList(new ClickHouseClientInstrumentation());
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggest to use static import here.

public class ClickHouseInstrumentationModule extends InstrumentationModule {

public ClickHouseInstrumentationModule() {
super("clickhouse", "clickhouse-client");
Copy link
Contributor

@steverao steverao Jul 1, 2024

Choose a reason for hiding this comment

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

According to previous convention, the module should be named to clickhouse-client-0.5. Please pay attention to modify other places together.

@@ -204,6 +204,7 @@ include(":instrumentation:cassandra:cassandra-4.4:library")
include(":instrumentation:cassandra:cassandra-4.4:testing")
include(":instrumentation:cassandra:cassandra-4-common:testing")
include(":instrumentation:cdi-testing")
include(":instrumentation:clickhouse:clickhouse-client:javaagent")
Copy link
Contributor

Choose a reason for hiding this comment

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

If there is a module, according to convention, please remove extra clickhouse directory. use clickhouse-client-0.5 directly.

@laurit laurit added this to the v2.6.0 milestone Jul 8, 2024
@trask trask merged commit 26591ea into open-telemetry:main Jul 9, 2024
56 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.

None yet

4 participants