[ci] Add Http client metrics. #3415
Conversation
apps/app/src/main/scala/org/lfdecentralizedtrust/splice/console/ConsoleHttpCommandRunner.scala
Outdated
Show resolved
Hide resolved
apps/app/src/main/scala/org/lfdecentralizedtrust/splice/console/ScanAppReference.scala
Outdated
Show resolved
Hide resolved
...common/src/main/scala/org/lfdecentralizedtrust/splice/config/CNRemoteParticipantConfig.scala
Outdated
Show resolved
Hide resolved
ray-roestenburg-da
force-pushed
the
ray/http-client-metrics
branch
3 times, most recently
from
632ad21 to
7749f68
Compare
|
/cluster_test |
|
Deploy cluster test triggered for Commit 7749f68cc59f67c87bc93c7a92c3f26ef106500b in , please contact a Contributor to approve it in CircleCI: https://app.circleci.com/pipelines/github/DACH-NY/canton-network-internal/44537 |
ray-roestenburg-da
force-pushed
the
ray/http-client-metrics
branch
3 times, most recently
from
8361b87 to
6ae7d99
Compare
|
/cluster_test |
|
Deploy cluster test triggered for Commit 6ae7d99e5eab1e198a8ae44ef9a41e99f5a7e181 in , please contact a Contributor to approve it in CircleCI: https://app.circleci.com/pipelines/github/DACH-NY/canton-network-internal/44546 |
ray-roestenburg-da
force-pushed
the
ray/http-client-metrics
branch
from
6ae7d99 to
16dc014
Compare
|
/cluster_test |
|
Deploy cluster test triggered for Commit 16dc014b327751024553429a7883c89ff4cb66ec in , please contact a Contributor to approve it in CircleCI: https://app.circleci.com/pipelines/github/DACH-NY/canton-network-internal/44578 |
Signed-off-by: Raymond Roestenburg <[email protected]>
ray-roestenburg-da
force-pushed
the
ray/http-client-metrics
branch
from
16dc014 to
1220027
Compare
|
Hey @nicu-da maybe you can have a first look, still need to add a dashboard and metrics tests though. |
nicu-da
left a comment
nicu-da
left a comment
There was a problem hiding this comment.
Thank you! Really nice cleanup.
I think we just need to tune the labels a bit more to get a proper dashboard built
| ) | ||
| } | ||
|
|
||
| def getParticipantClient()(implicit ec: ExecutionContext): ParticipantClientReference = { |
There was a problem hiding this comment.
we can make these private right?
There was a problem hiding this comment.
it's in abstract class so we can make it protected
apps/app/src/test/scala/org/lfdecentralizedtrust/splice/unit/http/HttpClientProxyTest.scala
Show resolved
Hide resolved
apps/common/src/main/scala/org/lfdecentralizedtrust/splice/http/HttpClient.scala
Outdated
Show resolved
Hide resolved
|
|
||
| object HttpClientMetrics { | ||
| def context(request: HttpRequest, path: String) = | ||
| MetricsContext("method" -> request.method.name, "path" -> path) |
There was a problem hiding this comment.
I am afraid that will blow up the cardinality. Issue is that we can have paths that also include ids in them and that can grow unbounded.
I would instead add an operationName param to the execute method, and propagate from the call sites to ensure we can track which method it is.
There was a problem hiding this comment.
We probably also want to include a clientName or something to distinguish between scan client, sv app client
There was a problem hiding this comment.
Ok, I wanted to avoid doing that since it's a bit more work, but I agree that it makes sense, and is worth the effort. (good point on blowing up the cardinality)
| } | ||
|
|
||
| object HttpClientMetrics { | ||
| def context(request: HttpRequest, path: String) = |
There was a problem hiding this comment.
Would it be feasible to also add the host?
There was a problem hiding this comment.
yes, will do (authority part of request)
…p/HttpClient.scala Co-authored-by: Nicu Reut <[email protected]> Signed-off-by: Raymond Roestenburg <[email protected]>
Pull Request Checklist
Cluster Testing
/cluster_teston this PR to request it, and ping someone with access to the DA-internal system to approve it./hdm_teston this PR to request it, and ping someone with access to the DA-internal system to approve it.PR Guidelines
Fixes #n, and mention issues worked on using#nMerge Guidelines