make counter classes public #12386
make counter classes public #12386
Conversation
| @@ -136,7 +136,7 @@ def reset(self) -> None: | |||
| self._count = 0 | |||
|
|
|||
|
|
|||
| class _CounterMetric(Metric, _Counter): | |||
| class CounterMetric(Metric, _Counter): | |||
There was a problem hiding this comment.
question: _Counter is still private as to the convention with the leading underscore. Would that mean that increment and reset are still considered private functions in the IDE integration / for type suggestions?
There was a problem hiding this comment.
The IDE suggestions seem to work the same as before, showing both increment and reset as public methods. It does feel odd to see a public method suggested from a class that is, by convention, private.
Making _Counter public would require renaming it to avoid conflicts with the factory class, and it would also implicitly allow direct usage, something I’d like to avoid.
Would making it public while explicitly stating in the docstring that it should not be used directly be an acceptable solution? It seems like the Counter factory class (Counter) is creating more ambiguity than value 🤔
There was a problem hiding this comment.
Well, that's totally yours to judge. As mentioned in the initial PR on the new metrics framework, I don't really see the need for the Counter class which "redirects to the right class" in the first place, but it's fair if you want to keep it this way. Also, as mentioned in the other comment, you can very much guide the usage of this framework, and - together with comments in the docstring - can drive the usage. Once it's used in a specific way all over the place it will be used in this way moving forward for new implementations as well usually.
If you really want to keep it like that you can still make the interface explicit and abstract while having a private class with the actual implementation (as outlined in my other comment).
That would bloat up the amount of classes, but would allow you to have everything you want:
- Create public abstract base classes defining the interfaces for both the
CounterMetric, theCounter, and theLabeledCounterMetric. - Create private implementations of each of the abstract classes.
- Use the public abstract classes as type hints for the Counter "factory", but the private implementations when creating them.
There was a problem hiding this comment.
Ok, agreed. In a separate PR over the next few days, I’ll either remove the factory class or implement abstract classes to define the interfaces. In the meantime, I’m adding comments in the docstrings to avoid blocking the other incoming PRs. This upcoming change won’t impact how the data is sent.
Additionally, introducing that change should be fairly straightforward for me and won’t require involving other engineers, as they’re already handling the migration from the old counter to the new one.
Thanks again for the feedback! 💪🏼 🙏🏼
| @@ -136,7 +136,7 @@ def reset(self) -> None: | |||
| self._count = 0 | |||
|
|
|||
|
|
|||
| class _CounterMetric(Metric, _Counter): | |||
| class CounterMetric(Metric, _Counter): | |||
There was a problem hiding this comment.
question: Making them public could mean that they are used directly instead of via the Counter init, Is that something that is fine, or should this be mentioned explicitly in the docstring maybe?
There was a problem hiding this comment.
Actually, this is something I’d like to avoid (having the classes used directly instead of the Counter). Do you think specifying this in the docstring would be sufficient?
There was a problem hiding this comment.
That depends, you can make it absolutely clear by creating abstract classes, where the actual implementations are private, but I don't think that would be necessary. You can add it to the docstring, and you can guide core engineers in the PRs when they migrate to the new metrics client. I think that should be enough for now. :)
Co-authored-by: Alexander Rashed <2796604+alexrashed@users.noreply.github.com>
…BaseCounter should not be instantiated directly
…BaseCounter should not be instantiated directly
Motivation
@bentsku noticed this behavior while working on this PR, thanks for the heads-up! 🚀
Before this change, the classes
_CounterMetricand_LabeledCounterMetricwere "protected", which made it inappropriate to use them directly for static typing. This caused type hinting issues, particularly in cases where a Counter instance was assigned to a class attribute:Changes
The
CounterMetricandLabeledCounterMetricclasses have been made public, allowing clients to use them directly for typing. The use of TypeVar and Protocol was also considered, but it may not be necessary to rely on them in this specific case.