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

Manually instrument okhttp with OTEL in trino-jdbc #17101

Merged
merged 1 commit into from
Jul 7, 2023

Conversation

nineinchnick
Copy link
Member

Description

Because trino-jdbc is using a shaded version of okhttp, it does not get automatically instrumented when using an OTEL Java agent.

Thanks to this, both client and server traces can be correlated, with JDBC traces as the root.

This is a follow-up to #16950.

Additional context and related issues

Release notes

(x) This is not user-visible or docs only and no release notes are required.
( ) Release notes are required, please propose a release note for me.
( ) Release notes are required, with the following suggested text:

@cla-bot cla-bot bot added the cla-signed label Apr 18, 2023
@nineinchnick nineinchnick requested a review from electrum April 18, 2023 11:47
@github-actions github-actions bot added the jdbc Relates to Trino JDBC driver label Apr 18, 2023
@electrum
Copy link
Member

We should copy the necessary attributes from SemanticAttributes, since that might not be present. See Semantic conventions for HTTP spans.

Instrumenting the HTTP client inside the driver should be sufficient for context propagation. For users who want higher level integration, we should see if the Library Instrumentation for JDBC using OpenTelemetryDataSource will work. We might need to enhance the instrumentation for Trino. Note that Trino is now included in db.system conventions.

@wendigo
Copy link
Contributor

wendigo commented Jul 6, 2023

Can you rebase this PR? I'll merge it when the automation finishes

@wendigo
Copy link
Contributor

wendigo commented Jul 6, 2023

You could also run the integration test that checks whether jdbc driver is shaded properly (i've added this test recently)

@wendigo
Copy link
Contributor

wendigo commented Jul 6, 2023

Cancelled build. The jdbc jar is not shaded properly:

creating: io/opentelemetry/
   creating: io/opentelemetry/semconv/
   creating: io/opentelemetry/semconv/trace/
   creating: io/opentelemetry/semconv/trace/attributes/
  inflating: io/opentelemetry/semconv/trace/attributes/SemanticAttributes$HttpFlavorValues.class
  inflating: io/opentelemetry/semconv/trace/attributes/SemanticAttributes$NetHostConnectionTypeValues.class
  inflating: io/opentelemetry/semconv/trace/attributes/SemanticAttributes.class
  inflating: io/opentelemetry/semconv/trace/attributes/SemanticAttributes$NetTransportValues.class
  inflating: io/opentelemetry/semconv/trace/attributes/SemanticAttributes$MessagingRocketmqMessageTypeValues.class
  inflating: io/opentelemetry/semconv/trace/attributes/SemanticAttributes$MessageTypeValues.class
  inflating: io/opentelemetry/semconv/trace/attributes/SemanticAttributes$FaasDocumentOperationValues.class
  inflating: io/opentelemetry/semconv/trace/attributes/SemanticAttributes$FaasInvokedProviderValues.class
  inflating: io/opentelemetry/semconv/trace/attributes/SemanticAttributes$RpcGrpcStatusCodeValues.class
  inflating: io/opentelemetry/semconv/trace/attributes/SemanticAttributes$RpcConnectRpcErrorCodeValues.class
  inflating: io/opentelemetry/semconv/trace/attributes/SemanticAttributes$RpcSystemValues.class
  inflating: io/opentelemetry/semconv/trace/attributes/SemanticAttributes$MessagingOperationValues.class
  inflating: io/opentelemetry/semconv/trace/attributes/SemanticAttributes$MessagingSourceKindValues.class
  inflating: io/opentelemetry/semconv/trace/attributes/SemanticAttributes$EventDomainValues.class
  inflating: io/opentelemetry/semconv/trace/attributes/SemanticAttributes$MessagingRocketmqConsumptionModelValues.class
  inflating: io/opentelemetry/semconv/trace/attributes/SemanticAttributes$DbCassandraConsistencyLevelValues.class
  inflating: io/opentelemetry/semconv/trace/attributes/SemanticAttributes$MessagingDestinationKindValues.class
  inflating: io/opentelemetry/semconv/trace/attributes/SemanticAttributes$OtelStatusCodeValues.class
  inflating: io/opentelemetry/semconv/trace/attributes/SemanticAttributes$NetHostConnectionSubtypeValues.class
  inflating: io/opentelemetry/semconv/trace/attributes/SemanticAttributes$NetSockFamilyValues.class
  inflating: io/opentelemetry/semconv/trace/attributes/SemanticAttributes$DbSystemValues.class
  inflating: io/opentelemetry/semconv/trace/attributes/SemanticAttributes$FaasTriggerValues.class
  inflating: io/opentelemetry/semconv/trace/attributes/SemanticAttributes$OpentracingRefTypeValues.class
  inflating: io/opentelemetry/semconv/trace/attributes/SemanticAttributes$GraphqlOperationTypeValues.class
  inflating: io/opentelemetry/semconv/trace/attributes/package-info.class
   creating: io/opentelemetry/semconv/resource/
   creating: io/opentelemetry/semconv/resource/attributes/
  inflating: io/opentelemetry/semconv/resource/attributes/ResourceAttributes$TelemetrySdkLanguageValues.class
  inflating: io/opentelemetry/semconv/resource/attributes/ResourceAttributes$OsTypeValues.class
  inflating: io/opentelemetry/semconv/resource/attributes/ResourceAttributes$AwsEcsLaunchtypeValues.class
  inflating: io/opentelemetry/semconv/resource/attributes/ResourceAttributes.class
  inflating: io/opentelemetry/semconv/resource/attributes/ResourceAttributes$CloudProviderValues.class
  inflating: io/opentelemetry/semconv/resource/attributes/ResourceAttributes$HostArchValues.class
  inflating: io/opentelemetry/semconv/resource/attributes/package-info.class
  inflating: io/opentelemetry/semconv/resource/attributes/ResourceAttributes$CloudPlatformValues.class
  inflating: io/opentelemetry/semconv/version.properties

@nineinchnick nineinchnick force-pushed the instrument-jdbc branch 2 times, most recently from 3c749ec to 05fcad1 Compare July 6, 2023 15:23
Because trino-jdbc is using a shaded version of okhttp, it does not get
automatically instrumented when using an OTEL Java agent.
@nineinchnick
Copy link
Member Author

All done, tested manually.

@wendigo
Copy link
Contributor

wendigo commented Jul 7, 2023

LGTM

@wendigo wendigo merged commit c5de866 into trinodb:master Jul 7, 2023
@wendigo
Copy link
Contributor

wendigo commented Jul 7, 2023

Thanks @nineinchnick !

@nineinchnick nineinchnick deleted the instrument-jdbc branch July 7, 2023 14:31
@github-actions github-actions bot added this to the 422 milestone Jul 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed jdbc Relates to Trino JDBC driver
Development

Successfully merging this pull request may close these issues.

3 participants