Define metrics with Prometheus integration #33
Conversation
Signed-off-by: Tim Li <[email protected]>
Signed-off-by: Tim Li <[email protected]>
Signed-off-by: Tim Li <[email protected]>
|
|
||
|
|
||
| # Default Cadence metrics names | ||
| class CadenceMetrics: |
There was a problem hiding this comment.
I don't think these should be in a class, maybe an enum if you want to namespace them.
Do these match the Java/Go client or are these just examples for the sake of testing?
There was a problem hiding this comment.
Thanks, I replaced them with enums.
|
|
||
|
|
||
| # Global default handler | ||
| _default_handler: Optional[MetricsHandler] = None |
There was a problem hiding this comment.
I don't think we should have a global for this, I'd rather we pass it into the Client which propagates to the Workers.
|
|
||
|
|
||
| @dataclass | ||
| class PrometheusConfig: |
There was a problem hiding this comment.
I'm not very familiar with Prometheus. Is the expectation that the user needs to configure all of this? How does the setup compare to the Java/Go clients and the server? They also all support Prometheus, right?
There was a problem hiding this comment.
The setup should be done at service start up, which is the same process if we want to use prometheus in java client
| except Exception as e: | ||
| logger.error(f"Failed to send histogram {key}: {e}") | ||
|
|
||
| def start_http_server(self) -> None: |
There was a problem hiding this comment.
Does the user need to call this and manage the lifecycle of this object?
There was a problem hiding this comment.
No, I change the behavior so that it will initialize when client start
Signed-off-by: Tim Li <[email protected]>
|
|
||
|
|
||
| # Default Cadence metrics names | ||
| class CadenceMetrics(Enum): |
| WORKFLOW_DURATION_SECONDS = "workflow_duration_seconds" | ||
|
|
||
| # Activity metrics | ||
| ACTIVITY_STARTED_TOTAL = "activity_started_total" |
There was a problem hiding this comment.
Are these metrics actual ones that exist? Do they match the Go/Java client?
| @@ -0,0 +1,12 @@ | |||
| """Visibility and metrics collection components for Cadence client.""" | |||
There was a problem hiding this comment.
MetricsEmitter and the prometheus stuff should be in a public package. Users need to specify it in the ClientOptions so it should be visibile. Maybe cadence/metrics
Additionally we should go with metrics as the package name. I don't think visibility accurately describes it.
| HISTOGRAM = "histogram" | ||
|
|
||
|
|
||
| class MetricsEmitter(Protocol): |
There was a problem hiding this comment.
Is this also the interface that users should use within Workflows or will we add something else?
There was a problem hiding this comment.
Yes, user will be using this in their workflow/activity
| """Configuration for Prometheus metrics.""" | ||
|
|
||
| # Metric name prefix | ||
| metric_prefix: str = "cadence_" |
There was a problem hiding this comment.
I don't think we actually want to expose this as an option unless our other clients do. It makes standard dashboards impossible.
There was a problem hiding this comment.
I will make this into a hardcoded constant to match other clients
Signed-off-by: Tim Li <[email protected]>
2 participants
What changed?
Why?
It enables metrics collection for monitoring Cadence workflows and activities
How did you test it?
unit tests
Potential risks
Release notes
Documentation Changes