Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add variadic arguments to metric exporter/reader interfaces #2654

Merged
merged 5 commits into from
May 4, 2022

Conversation

ocelotl
Copy link
Contributor

@ocelotl ocelotl commented May 1, 2022

Fixes #2650

@ocelotl ocelotl requested a review from a team as a code owner May 1, 2022 22:42
@ocelotl ocelotl self-assigned this May 1, 2022
@lzchen
Copy link
Contributor

lzchen commented May 2, 2022

What about adding kwargs to the constructors of all these public components?

@ocelotl
Copy link
Contributor Author

ocelotl commented May 2, 2022

What about adding kwargs to the constructors of all these public components?

Not sure if that is necessary, the idea is that we need kwargs to receive extra arguments that we may decide to add in the future to an existing method call that we have now. For example here we have a call to shutdown. If in the future we want to pass an extra argument to that call, we need the underlying exporter stored in self._exporter there to have a method shutdown that can receive extra arguments. We don't have calls that instantiate these classes in our existing code so, I don't think we need to add them to the constructors.

I guess in the future we could have that constructor call and in theory every method could be in this situation, so the "right" policy would be to add kwargs to all methods, but I don't think we want that.

@lzchen
Copy link
Contributor

lzchen commented May 2, 2022

@ocelotl

the idea is that we need kwargs to receive extra arguments that we may decide to add in the future to an existing method call that we have now

Is that what the original issue is about? The original issue says add args and kwargs to "user implementable interfaces". The constructors should be part of that no?

I guess in the future we could have that constructor call and in theory every method could be in this situation, so the "right" policy would be to add kwargs to all methods, but I don't think we want that.

I think this is what we want right? We want to have the flexibility to change the signature of every user exposed interface, including methods.

@ocelotl
Copy link
Contributor Author

ocelotl commented May 2, 2022

@ocelotl

the idea is that we need kwargs to receive extra arguments that we may decide to add in the future to an existing method call that we have now

Is that what the original issue is about? The original issue says add args and kwargs to "user implementable interfaces". The constructors should be part of that no?

If that is the case we should mark the constructor with abstractmethod.

@ocelotl ocelotl requested review from lzchen and codeboten May 2, 2022 20:22
This reverts commit 7eb6134.
@ocelotl ocelotl force-pushed the issue_2650 branch 2 times, most recently from 21a8ae1 to 6bf8feb Compare May 4, 2022 20:13
@ocelotl ocelotl merged commit 7397605 into open-telemetry:main May 4, 2022
@ocelotl ocelotl deleted the issue_2650 branch May 4, 2022 20:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add *args, **kwargsto user implementable interfaces
5 participants