Metric meter support #177
Metric meter support #177
Conversation
# Conflicts: # temporalio/test/sig/test.rbs # temporalio/test/test.rb
Sushisource
left a comment
Sushisource
left a comment
There was a problem hiding this comment.
LGTM, just one suggested change
| # @param opentelemetry [OpenTelemetryMetricsOptions, nil] OpenTelemetry options if using OpenTelemetry. This is | ||
| # mutually exclusive with `prometheus`. | ||
| # @param prometheus [PrometheusMetricsOptions, nil] Prometheus options if using Prometheus. This is mutually | ||
| # exclusive with `opentelemetry`. |
There was a problem hiding this comment.
Don't really love that. Can we just have two methods?
There was a problem hiding this comment.
This is a constructor and Ruby does not have overloads so you can't separate out the methods unless you made non-constructor static helpers which I don't think we'd want here. One thing we can do is have one union option. This is what we do in Python, but in .NET for typing reasons we have separate mutually exclusive fields. Granted Python brought these "metrics" specific options up to the Telemetry layer which we shouldn't do again, so they got to call this field metrics: Optional[Union[OpenTelemetryConfig, PrometheusConfig, MetricBuffer]]. If we combine for Ruby, what should we call the field? Surely not "metrics" (this is already inside of "metrics"). Open to names here. Otherwise, I think the mutually exclusive options like .NET is fine.
There was a problem hiding this comment.
Static helpers would be OK by me I think, but, if that's atypical we don't have to
There was a problem hiding this comment.
I think it's a bit atypical. Having mutually exclusive kwargs is pretty common. MetricsOptions.new(opentelemetry: whatever) or MetricsOptions.new(prometheus: whatever) reads well.
2 participants
What was changed
Temporalio::Metric::MeterandTemporalio::Metricand supporting structuremetric_meterinTemporalio::RuntimeandTemporalio::Activity::Contextfor use by usersChecklist