feat: Expose protected attributes and methods (only create*Widget) of all monitoring classes to allow users to build custom monitoring more flexibly via composition rather than inheritance.

[Dicee]

feat: Expose protected attributes and methods (only create*Widget) of all monitoring classes to allow users to build custom monitoring more flexibly via composition rather than inheritance.

See #218

---

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

[echeung-amzn]

Requires a rebase. It'd be nice to have a example unit test similar to what's given in #218 too, but that can be in a separate PR.

[niebloomj]

Do you need help resolving merge conflicts?

[Dicee]

Thanks @echeung-amzn ! I'll look into adding a unit test. @niebloomj thanks for offering, if you have permissions feel free to help, I see there are a little more than 30 conflicts in just one file. What I've done shouldn't really impact this file so I think I'll just take the version from main for every conflict.

[niebloomj]

Thanks @echeung-amzn ! I'll look into adding a unit test. @niebloomj thanks for offering, if you have permissions feel free to help, I see there are a little more than 30 conflicts in just one file. What I've done shouldn't really impact this file so I think I'll just take the version from main for every conflict.

I probably don't have permissions lol I just really want this to get pushed through so want to offer whatever I can do to help.

[echeung-amzn]

LGTM, but will wait for a second approval before merging since it's a relatively impactful change.

[Dicee]

Thanks for the support @echeung-amzn !

[ayush987goyal]

LGTM. One callout though, this will expose a lot of internal API which would mean it would be difficult to make opinionated changes in the future. We need to be careful on such changes to ensure that we don't break customers. With great power comes great responsibility!

[Dicee]

Makes sense, though as already argued in the corresponding issue, this was already exposed via inheritance, and already leveraged by clients! Thanks all for the reviews and support.

[niebloomj]

I'd like to see a migration document for how we want to handle breaking changes and deprecation warnings given this exposure, even though the exposure was technically already there.

[Dicee]

Since they're all very small pieces of logic that are completely independent from one another, I expect it will be quite a simple deprecation strategy. I say deprecation because it's the only kind of relevant change, since the existing API already exposes (and quite dramatically) clients to behavior changes. If anything, people using these methods will be pleased to know their dashboard cannot dramatically change from one version to the next, while those fully relying on preset decisions made for them will have to accept these decisions can change under their feet.

So for deprecation, I was saying, I'd say it's just a matter of marking the field or method as deprecated, and eventually removing it. Since they are independent, having some methods lingering for longer than desired won't be too much of an annoyance as it won't prevent modifying the rest of the code as needed. Overall, I don't feel very concerned about this but let me know if I'm over-simplifying :)

[voho]

I am still bit on fence with this, since it will basically expose everything... but if the users find this useful and we can leverage it as well, e.g. for building composition-based structures, lets go with it.

Apart from this, I am more worried about missed monitoring classes and conflicts - are you confident enough we covered everything? We will have to make one more sweek to make sure we did not miss any class and do the same for the internal library.

[Dicee]

Thanks for the review! I think I went through all relevant classes, yes. On "conflicts", I'm not sure what you have in mind, could you detail your concern?

[Dicee]

Can someone with write access please help pushing this?

[Dicee]

Thanks @ayush987goyal