Custom metric support #384
Conversation
|
Could you add a link in the PR description to the GitHub / Jira issue that motivates the changes? |
I linked this PR from that issue: #369. I will update the description here. |
| # sync function and we don't support cross-process metrics | ||
| if not self.runtime_metric_meter: | ||
| raise RuntimeError( | ||
| "Metrics meter not available in non-threaded sync activities like mulitprocess" |
There was a problem hiding this comment.
| "Metrics meter not available in non-threaded sync activities like mulitprocess" | |
| "Metrics meter not available in non-threaded sync activities like multiprocess" |
| if not ref: | ||
| return None | ||
| return MetricMeter(ref) |
There was a problem hiding this comment.
Just a personal Python style suggestion, obviously FFTI.
| if not ref: | |
| return None | |
| return MetricMeter(ref) | |
| return MetricMeter(ref) if ref else None |
There was a problem hiding this comment.
I try to avoid the RHS ternary in some of these cases, but you'll see I use it in others. I may use it here.
| for k, v in at_least_labels.items(): | ||
| if not f'{k}="{v}"' in line: | ||
| return False | ||
| return line.endswith(f" {value}") |
There was a problem hiding this comment.
More small-minded personal style suggestions while trying to digest the PR, FFTI!
| for k, v in at_least_labels.items(): | |
| if not f'{k}="{v}"' in line: | |
| return False | |
| return line.endswith(f" {value}") | |
| return all( | |
| f'{k}="{v}"' in line for k, v in at_least_labels.items() | |
| ) and line.endswith(f" {value}") |
There was a problem hiding this comment.
Yeah, it's style preference. I do use any/all where I do think functional approach has more value.
There was a problem hiding this comment.
Yes, here I would personally say that all(...) is more Pythonic than explicitly implementing an early-exit algorithm.
There was a problem hiding this comment.
👍 (just test code, and this style for assert is clearer with pytest because of how it expands the expression, but no assert in here so can change)
| """Initialize gauge.""" | ||
| self._ref = meter._ref.new_gauge(name, description, unit) | ||
|
|
||
| def set(self, value: int, attrs: MetricAttributes) -> None: |
There was a problem hiding this comment.
Isn't value supposed to be a float here?
There was a problem hiding this comment.
There was a problem hiding this comment.
Looks like otel metrics support both floats and ints for all metric types.
I think we should too.
https://github.com/open-telemetry/opentelemetry-python/blob/main/opentelemetry-api/src/opentelemetry/metrics/_internal/instrument.py#L319
https://github.com/open-telemetry/opentelemetry-go/tree/main/metric
There was a problem hiding this comment.
Opened an issue in sdk-core temporalio/sdk-core#604
There was a problem hiding this comment.
I don't think that should be a blocker for this PR
What was changed
MetricMeter,MetricCounter,MetricHistogram, andMetricGaugeas (abstract) classes totemporalio.commonmetric_meterproperty totemporalio.runtime.Runtimetemporalio.workflow.metric_meter()function to get replay-safe metric metertemporalio.activity.metric_meter()function to get optional metric metermetric_meterattribute totemporalio.testing.ActivityEnvironmentthat defaults to noop implHandles part of #369