Make MetricsRegistry as an interface #12487
Conversation
|
|
There was a problem hiding this comment.
To make sure I understand, the idea here is for Cloud to implement the MetricsRegistry interface?
Looks like there are some access modifiers errors that are breaking the build: https://github.com/airbytehq/airbyte/runs/6236766028?check_suite_focus=true#step:9:1274
| * <p> | ||
| * - Include the time period in the name if the metric is meant to be run at a certain interval. | ||
| */ | ||
| public enum MetricsRegistries implements MetricsRegistry { |
There was a problem hiding this comment.
nit: Instead of MetricsRegistries as the enum name, what about MetricsRegistryEnum or MetricsRegistryImpl? The plural "registries" feels slightly off to me
There was a problem hiding this comment.
What about PublicMetricRegistry or OssMetricRegistry? The practice of adding the java type to the name is a throwback to when IDEs didn't provide introspection. I generally feel its superfluous and make it more difficult to read.
There was a problem hiding this comment.
Sounds good - I was following MetricEmittingApp & MetricEmittingApps pattern. I think the Oss one makes sense.
There was a problem hiding this comment.
makes sense. i suppose that pattern is less logical here since it suggests we have multiple registries. appreciate trying to keep the consistency!
Exactly. Think some metrics such as orb are not meant to be exposed to public. Sorry about the errors, I'll address them. |
github-actions
bot
added
area/platform
Make MetricsRegistry as an interface
What
Make MetricsRegistry as an interface so cloud server could extend from it, because cloud server has its own metrics that is not relevant to OSS
How
Make MetricsRegistry an interface so cloud server could use and implement an enum class out of it.
Recommended reading order
x.javay.python🚨 User Impact 🚨
Nope