chore: support OpenTelemetry metrics #1465
Conversation
| @@ -98,6 +110,16 @@ public BackendConnection getBackendConnection() { | |||
| return backendConnection; | |||
| } | |||
|
|
|||
| public static Attributes getMetricAttributes(DatabaseId databaseId) { | |||
| AttributesBuilder attributesBuilder = Attributes.builder(); | |||
| if (databaseId != null) { | |||
There was a problem hiding this comment.
databaseId should never be null (at least not for a real connection, maybe in some tests it is). Could we therefore change this into a more direct implementation, where:
metricAttributesare always created whenExtendedQueryProtocolHandleris created.- This method is not static, but an instance method, and always just returns the actual
metricAttributesof thisExtendedQueryProtocolHandler. It should also not takeDatabaseId(or anything else) as an input argument, as theDatabaseIdis fixed for anExtendedQueryProtocolHandler. The structure here is that: - Each connection from a client to PGAdapter has one
ConnectionHandler. - There is a one-to-one relationship between
ConnectionHandlerandExtendedQueryProtocolHandler. - There is a one-to-one relationship between
ExtendedQueryProtocolHandlerandBackendConnection. - All of the above classes also have exactly one
DatabaseIdthat never changes during their lifetime.
Now it seems like metricAttributes sometimes needs to be created on-the-fly, but I don't think that is ever true.
There was a problem hiding this comment.
Yes, I was originally creating metricAttributes in ExtendedQueryProtocolHandler and pass it to the BackendConnection. But I thought we already pass databaseId into it in here. I need to construct the attributes in Execute of BackendConnection.
There was a problem hiding this comment.
I also think databaseId should not be null but I add this check due to an error. If I add metricAttributes as an instance method, can the Execute of BackendConnection access it? Let me explore it more.
There was a problem hiding this comment.
Please take a look my latest changes. Basically, I need the attributes in Execute of BackendConnection.
src/main/java/com/google/cloud/spanner/pgadapter/utils/Metrics.java
Outdated
Show resolved
Hide resolved
src/main/java/com/google/cloud/spanner/pgadapter/utils/Metrics.java
Outdated
Show resolved
Hide resolved
|
@olavloite , currently, |
…pter into support-latency-metrics
hengfengli
changed the title
Simplify OpenTelemetry code by moving the Tracer and Metrics to BackendConnection. This reduces the need to manage the same objects in multiple places, and reduces the number of places where arguments need to be added to existing methods.
|
@hengfengli I took the liberty of doing a small refactor of the code in order to simplify things a bit. I think it is better if we only keep the tracer and metrics in one place (BackendConnection). In addition, we do not need to create a new |
Yes, that should be included as well. |
|
|
||
| PGAdapter supports Open Telemetry tracing. You can enable this by adding the `-enable_otel` command | ||
| line argument when starting PGAdapter. | ||
| PGAdapter supports OpenTelemetry tracing and metrics. You can enable the tracing or the metrics |
There was a problem hiding this comment.
nit:
| PGAdapter supports OpenTelemetry tracing and metrics. You can enable the tracing or the metrics | |
| PGAdapter supports OpenTelemetry tracing and metrics. You can enable tracing or metrics |
| * `spanner/pgadapter/client_lib_latencies`: Latency when the Spanner's client library receives | ||
| a call and returns a response. |
There was a problem hiding this comment.
| * `spanner/pgadapter/client_lib_latencies`: Latency when the Spanner's client library receives | |
| a call and returns a response. | |
| * `spanner/pgadapter/client_lib_latencies`: Latency between the Spanner client library receiving | |
| a request and returning a response. |
| GoogleCloudMetricExporter.createWithConfiguration( | ||
| MetricConfiguration.builder() | ||
| // Configure the cloud project id. | ||
| .setProjectId(projectId) |
There was a problem hiding this comment.
This needs a null check, as projectId can be null, and .setProject will throw a NullPointerException if the project is null.
projectId is null if PGAdapter has been started without a specific project on the command line.
olavloite
merged commit 1ed11f9
into
GoogleCloudPlatform:postgresql-dialect
Add two latency metrics:
spanner/pgadapter/roundtrip_latencies: measure the roundtrip latency in PGAdapter that can include multiple messages between the client and PGAdapter.spanner/pgadapter/client_lib_latencies: measure the time to make a request and get a response from the client library.