NamespaceMetricCapture in Nexus tests#9938
Conversation
stephanos
force-pushed
the
stephanos/nexus-new-metrics
branch
3 times, most recently
from
ed9af24 to
1655e0e
Compare
| env *NexusTestEnv, | ||
| url string, | ||
| completion nexusrpc.CompleteOperationOptions, | ||
| ) (map[string][]*metricstest.CapturedRecording, error) { |
There was a problem hiding this comment.
Callers track metrics themselves now because they need to distinguish between namespace vs global
There was a problem hiding this comment.
nice! It does look better to have the caller do the metric stuff rather than scattering part of it here
stephanos
force-pushed
the
stephanos/nexus-new-metrics
branch
2 times, most recently
from
bfc5322 to
da88682
Compare
stephanos
force-pushed
the
stephanos/nexus-new-metrics
branch
from
da88682 to
b8aaae9
Compare
|
|
||
| func (s *NexusWorkflowTestSuite) sendNexusCompletionRequest( | ||
| ctx context.Context, | ||
| env *NexusTestEnv, |
There was a problem hiding this comment.
Not needed anymore
|
|
||
| s.Run("NamespaceNotFoundNoIdentifier", func(s *NexusWorkflowTestSuite) { | ||
| env := s.newNexusWorkflowTestEnv(chasmEnabled) | ||
| env := s.newNexusWorkflowTestEnv(chasmEnabled, testcore.WithDedicatedCluster()) |
There was a problem hiding this comment.
Added wherever StartGlobalMetricCapture was used
| } | ||
|
|
||
| func (s *NexusAPIValidationTestSuite) TestNexusStartOperation_WithNamespaceAndTaskQueue_NamespaceNotFound() { | ||
| env := newNexusTestEnv(s.T(), false, testcore.WithDedicatedCluster()) |
There was a problem hiding this comment.
Not needed as using StartNamespaceMetricCapture
|
|
||
| testFn := func(s *NexusApiTestSuite, tc testcase, dispatchOnlyByEndpoint bool) { | ||
| env := newNexusTestEnv(s.T(), useTemporalFailures, testcore.WithDedicatedCluster()) | ||
| env.GetTestCluster().Host().SetOnAuthorize(func(ctx context.Context, c *authorization.Claims, ct *authorization.CallTarget) (authorization.Result, error) { |
There was a problem hiding this comment.
maybe a comment around here mentioning we still need dedicated cluster because of these calls (since some other parameterized tests are now done without dedicated cluster thanks to the new metric capture)
tests/nexus_api_validation_test.go
Outdated
| ctx := testcore.NewContext() | ||
| capture := env.GetTestCluster().Host().CaptureMetricsHandler().StartCapture() | ||
| defer env.GetTestCluster().Host().CaptureMetricsHandler().StopCapture(capture) | ||
| capture := env.StartNamespaceMetricCapture().ForNamespace(namespace) |
There was a problem hiding this comment.
I get the intent is to find the metric tagged w/ the nonexistent namespace, but it seems a bit awkward to create a metric capture for namespace-1 and then immediately filter by namespace-2.
I thought about this a bit, I think we can also do:
StartGlobalMetricCapture()and then later filter by namespace. I think this is fine aside from the panic in metric_capture.go if all metric are namespace-scoped. Maybe not preferable if you want to keep the panic to nudge people to use the right scope of capture- Maybe a
StartNamespaceMetricCapture(namespace string)function that allows you to capture from a specific namespace (or namespaces?) at capture start. I think it might read less awkward than starting capture in namespace-1 and then filter by namespace-2
There was a problem hiding this comment.
That's a good call, there's only one use case right now but let's add StartNamespaceMetricCaptureFor to allow passing a custom namespace in (Go doesn't allow method overloading) as that's def clearer.
| env *NexusTestEnv, | ||
| url string, | ||
| completion nexusrpc.CompleteOperationOptions, | ||
| ) (map[string][]*metricstest.CapturedRecording, error) { |
There was a problem hiding this comment.
nice! It does look better to have the caller do the metric stuff rather than scattering part of it here
2 participants
What changed?
Use the new NamespaceMetricCapture etc in Nexus tests.
Why?
Allows to remove a few dedicated cluster options; ie run faster.
How did you test it?